< phantomcircuit>
are we using git submodule or git subtree ?
< jonasschnelli>
phantomcircuit: subtree
< jonasschnelli>
secp256k1, univalue, leveldb
< nsh>
jgarzik, is univalue de novo code or based on another library?
< nsh>
(parsers scare me from a software security perspective. i wonder if it should be fuzzed or otherwise audited)
< gmaxwell>
it's denovo. it was originally vulnerable when it was merged in bitcoin core.
< gmaxwell>
after the first vulnerability was found extensive fuzz testing was done.
< gmaxwell>
Andytoshi implemented a functional equivilent in rust so we could do cross implementation round-trip/agreement tests, and a number of non-security bugs were found in it which haven't been fixed yet.
< nsh>
ah, great
< nsh>
i scanned the respository for the words 'grammar' 'lexer' and 'parser' and was disappointface but not surpriseface :)
< jonasschnelli>
univalue was used <0.12 for bitcoin-tx ... and its the default json parser/encode since 0.12
< jonasschnelli>
And by default I think there are no attack vectors for the JSON parser/encode...because you can't parse/encode unless you can do RPC auth or unless you enabled -rest=1
< gmaxwell>
nsh: fortunately we also do not take json from untrusted inputs.
< nsh>
right
< gmaxwell>
(even with rest=1)
< gmaxwell>
so that helps too.
< nsh>
there may be some instances where people are running services that expose the RPC to unvalidated input
< nsh>
even though this would be inadvisable. we can't really see how people use bitcoind in their services easily
< gmaxwell>
yea, maybe but I caution against that.
< * nsh>
nods
< gmaxwell>
I would be kind of surprised if there werent vulnerabilities there beyond the obvious.
< nsh>
likewise
< gmaxwell>
(obvious being that the walletexport/backup rpcs that take files can overwrite anything bitcoind can write to.)
< gmaxwell>
there are almost certantly memory exhaustion attacks one can perform from the RPC.
< nsh>
in principle bitcoind could be containerised to only have write access to the leveldb and config files it needs to overwrite
< nsh>
but that might not be proportionate
< nsh>
(and container solutions are not mature yet)
< gmaxwell>
it doesn't overwrite its own config files.
< nsh>
ah, my bad
< phantomcircuit>
nsh, backupwallet can write anywhere
< phantomcircuit>
which makes building a selinux profile mostly useless
< * nsh>
nods
< phantomcircuit>
etc etc for containerization
< nsh>
what's a sensible way forward in terms of least privilegeing [if that can be granted as a verb]
< nsh>
?
< jonasschnelli>
phantomcircuit: In case you want to check my first logdb implementation, check: https://github.com/libbtc/libbtc/pull/41 and feel free to feedback
< gmaxwell>
phantomcircuit: warren has been working on selinux policy that just denies that external FS access, and there is an selinux bool to reenable it if you really want it to be able to access outside of its directory.
< gmaxwell>
phantomcircuit: ultimately I think we should add replacements for those rpcs that send the data over the rpc... and depricate the ones that write locally.
< phantomcircuit>
gmaxwell, that sounds like a sound plan
< GitHub107>
[bitcoin] MarcoFalke opened pull request #7661: [wallet] Round up to the next satoshi on odd fee rates (master...Mf1603-walletCeil) https://github.com/bitcoin/bitcoin/pull/7661
< morcos>
I'm not sure that really counts though, who cares if the defniition of IsDust changes microscopically. It already changed drastically from 0.11.1 to 0.11.2 and back for 0.12.0
< morcos>
MarcoFalke: yeah i read that bullet point... is that even how the code works now?
< MarcoFalke>
if (nFeeNeeded == 0) check is still there
< morcos>
yeah
< morcos>
but that seems intended behavior right
< morcos>
at least if you change it round (neither ceil nor trunc)
< MarcoFalke>
If I `settxfee 1e-8`, I expect the wallet to go into the paytxfee-branch
< morcos>
if you say you want to pay 1 sat per 1000 bytes, and you put something of 499 bytes in there. why should we intrepet that as you intending to pay 1 satoshi and not 0
< morcos>
we already don't let you pay 0... so if you specify 0 another way, why is that different
< morcos>
i don't really care, but i just think the CFeeRate class needs to not get more convoluted
< morcos>
its already an annoying class
< MarcoFalke>
In the first case you pay fallbackfee, in the other case you pay 1 sat
< MarcoFalke>
It's mostly not relevant for mainnet
< morcos>
but you asked to pay 0
< morcos>
make it do ceiling then, i agree we need to just look through the follow on effects
< morcos>
but i don't see a priori why they would be a problem
< MarcoFalke>
You asked to pay 1 sat per kB and the wallet should always pay that fee rate (or slightly more)
< MarcoFalke>
Ok, I had the ceil-only patch somwhere laying around this mornig but I deleted it in the meantime
< morcos>
so are you ok with just making it always ceiling then?
< MarcoFalke>
I will just create a pull and see what others think
< morcos>
anyway, got to run, i don't like making that silly class more complicated though
< wumpus>
back in NL
< sipa>
congrats!
< btcdrak>
wumpus: \o/ now for jetlag sleep :)
< Bootvis_>
welcome back
< wumpus>
thanks :)
< jonasschnelli>
wumpus: welcome back, hope you had a good flight!
< sipa>
i'll be back in europe on friday
< jonasschnelli>
sipa: still in Boston?
< sipa>
sf now
< jonasschnelli>
hah. Globetrotter.
< morcos>
jonasschnelli: missed you in boston, we were about to split the wallet out from Core, but decided to push it off another 5 years since you weren't there
< sipa>
haha
< Luke-Jr>
lol
< morcos>
sipa: did you see my other comment on versionbits. about setting block version = 4?
< morcos>
i think it would make sense to also include in the first version bits soft fork a requirement that block version > 4
< morcos>
then the warning code can ignore blocks <= 4 because they will be invalid once the soft fork activates. this is analogous to how ISM soft forks have worked
< morcos>
we should also start setting TOP_BITS immediately... yes that means that old code will be warned to upgrade potentially before any soft fork is being rolled out, but that to me only serves to be more warning time.
< morcos>
if we were going to roll out verison bits code without the soft fork included, then i could see the argument against, b/c they'd get tricked into upgrading twice. but as long as they only need to upgrade that first time, its only a good thing that they are warned about it earlier
< jonasschnelli>
morcos: haha!
< jonasschnelli>
morcos: You and Suhas where there?
< morcos>
yeah, just came for the day monday
< jonasschnelli>
To bad I had to fly home on sunday.
< cfields>
jonasschnelli: you didn't come home and find a new baby, i hope? :)
< jonasschnelli>
cfields: Not yet. Still a couple of month to go...
< GitHub95>
bitcoin/master 9988554 R E Broadley: No "Unknown command" for getaddr command.
< GitHub95>
bitcoin/master c8d2473 Pieter Wuille: Merge #7642: Avoid "Unknown command" messages when receiving getaddr on outbound c…...
< GitHub152>
[bitcoin] sipa closed pull request #7642: Avoid "Unknown command" messages when receiving getaddr on outbound c… (master...GetAddrUnknownCommand) https://github.com/bitcoin/bitcoin/pull/7642
< cfields>
sipa: i managed to process where we were mis-aligned wrt threading models. I was far too close to the code to see what you were asking. I'll add some notes to the design doc to explain.
< sipa>
cfields: please don't let my comments cause you extra work
< sipa>
i think the model you used is perfect as a lowest-layer replacement for the current network code, and we should work to get it in
< cfields>
sipa: tl;dr: the event loop is essentially a large wrapper around select/epoll/etc, so thread management must be done a layer above
< cfields>
sipa: no problem, i just want to be able to explain the design choices, so that discussion was helpful.
< GitHub45>
[bitcoin] sipa opened pull request #7663: Make the generate RPC call function for non-regtest (master...generatenonreg) https://github.com/bitcoin/bitcoin/pull/7663
< gmaxwell>
My analysis on #1643 was that the change suggested there would potentially cause wallets to be more ground up into small coins; the author there did some simulation and and it least some models saw that effect. (doubling the utxo set size in test case 7, and increasing it by 40% in test case 8).
< gmaxwell>
And now we have a user complaining about the effect.
< gmaxwell>
I missed that we merged this.... I think immediate action may be needed to improve the situation, explosion in the utxo set size may take a long time to reverse.
< gmaxwell>
sipa: basically the old behavior didn't do what it said on the tin, the coin selection would sometimes select extra coins that it technically didn't have to select.
< gmaxwell>
So the change fixed that to prune them out.
< gmaxwell>
and I'm wondering if this wasn't responsible for the increase we've seen in the utxo growth rate even after filtering out some of the obvious dust-spam.
< sipa>
perhaps we should make the wallet produce 2 change outputs, where one mimicks the amount sent (mimcks, not equals)
< sipa>
that's for privacy, not for utxo optimization, but i expect it to lead to a better distribution of coin sizes to pick from
< gmaxwell>
sipa: I've suggested a scheme before where you do two change outputs and flip a coin: either you split the change amount, OR you copy the output amount.
< gmaxwell>
(though I suggested doing this only when the change would be fairly large)
< gmaxwell>
in general I think we should have a scheme which does not select coins but selects 'taint groups' (maybe a taint group is just a particular scriptpubkey) and spends all the coins in it, subject to some sanity limitation.
< cfields>
sipa: re our aes impl. Would you prefer to remove the tables before merge? Or ok to do it as a next step?
< cfields>
(assuming it wouldn't be worse than what we currently have with openssl)
< cfields>
sipa: by which i mean "would you prefer that i remove the tables" ofc :)
< sipa>
cfields: i'm now thinking about how you would implement all of the AES steps without tables
< gmaxwell>
AES is notoriously painful to do both sidechannel free and fast.
< gmaxwell>
The tables could be converted into CMOV oblivious lookups, for a big speed hit.
< cfields>
sipa: ah, our discussion had me believe that there was a standard alternate approach
< sipa>
cfields: hmm, no; i think you'll find plenty of academic work for fast & sidechannel free AES, but nothing standard
< sipa>
*but* i think AES is trivial to do slow & sidechannel free
< sipa>
well, constant-time
< sipa>
hmm, the S-box is nontrivial
< sipa>
i guess you'll need a cmov for the sbox
< sipa>
everything else can be done algebraically... the first thing besides the S-box is a GF(2^8) multiplication by 3
< sipa>
s/first thing/worst thing/
< morcos>
gmaxwell: re: #4906, i flagged that shortly after it got merged echoing your concerns. wumpus asked us if we should revert it and you and i both told him no.
< gmaxwell>
lol
< morcos>
gmaxwell: i think its unlikely that its had much of an effect on utxo set already, b/c its only been included since 0.12 right? but this is an example of something that we'll all forget about revisiting in time for 0.13 if we don't put it on a list somewhere
< gmaxwell>
morcos: it's been merged for a while, so it may be the case that people running pre-releases have had an effect.
< gmaxwell>
certantly if it is the cause of this user's complaint it's a cause for concern.
< gmaxwell>
But I think we wouldn't revert regardless, we would move forward.
< morcos>
Should we maintain an issue that includes goals for 0.13, or open a new issue and tag it with milestone 0.13 or something.. it'd be nice if we were more organized about this stuff
< gmaxwell>
I think perhaps one reason I also might have said to not revert it was a belief that we'd move forward and further fix the issue before the release but then it fell off the radar. A milestoned issue would probably be fine.