< bitcoin-git>
bitcoin/master 9429a39 practicalswift: Handle rename failure in DumpMempool(...) by using RenameOver(...) return ...
< bitcoin-git>
bitcoin/master ce9dd45 practicalswift: Add [[nodiscard]] to RenameOver(...)
< bitcoin-git>
bitcoin/master ffd5e7a MarcoFalke: Merge #20519: Handle rename failure in DumpMempool(...) by using the Renam...
< bitcoin-git>
[bitcoin] MarcoFalke merged pull request #20519: Handle rename failure in DumpMempool(...) by using the RenameOver(...) return value. Add [[nodiscard]] to RenameOver(...). (master...renameover-nodiscard) https://github.com/bitcoin/bitcoin/pull/20519
< bitcoin-git>
[bitcoin] naumenkogs opened pull request #20539: Avoid rebucketing on restart when it's not necessary (master...2020-12-01-rebucket-asmap-fix) https://github.com/bitcoin/bitcoin/pull/20539
< MarcoFalke>
michaelfolkson: Let's take this here
< michaelfolkson>
MarcoFalke: Sure, thanks
< michaelfolkson>
I think I'm misunderstanding terminology. So seeds will be randomly generated...
< MarcoFalke>
Some fuzz engines have command line switches to make them deterministic
< michaelfolkson>
But if a fuzz tester finds an issue with a randomly generated seed a different fuzz tester who tries that same input needs to get the same result
< MarcoFalke>
yes
< MarcoFalke>
if you can't reproduce, it will be hard to debug and fix
< michaelfolkson>
But you can reproduce as long as you know what the inputs were. Whether they were randomly generated or not is surely irrelevant?
< michaelfolkson>
It is surely a logging issue rather than a choice of deterministic/random?
< MarcoFalke>
int main() { if (rand() > 2) crash(); }
< MarcoFalke>
how would you reliably reproduce a crash here, knowing the input?
< MarcoFalke>
how the input was generated is irrelevant to how to reproduce a bug
< MarcoFalke>
logging helps to debug intermittent (non-reproducible) bugs better
< MarcoFalke>
we have a lot of them, which is why logging is cranked up in all tests
< michaelfolkson>
In your example one fuzz tester would crash the program with say an input of 3. For another fuzz tester to reproduce it they would need to know the other fuzz tester tried an input of 3
< michaelfolkson>
But how the fuzz tester initially generated 3 is irrelevant. It could have been deterministic or random
< michaelfolkson>
So in the PR there is a CAddrMan fuzzing harness. Again I'm not entirely sure how "harness" is defined
< michaelfolkson>
But fuzz testers should be able to randomly generate addrs and try different addrs to other fuzz testers. As long as they log which addrs they tried and which addrs created issues?
< MarcoFalke>
whether the above example crashes is *independent* of the input
< MarcoFalke>
the progam takes no input, assuming that rand will ask the OS for randomness
< MarcoFalke>
and in most cases the OS randomness is not reproducible
< michaelfolkson>
And the problem is that we don't log what the OS supplied for randomness?
< MarcoFalke>
the problem is that we ask the os for randomness
< MarcoFalke>
tests should supply the randomness. Either with a hardcoded seed or letting the fuzz engine pick one
< michaelfolkson>
Ah so the problem is an external source of randomness rather than an internal source from within the tests
< michaelfolkson>
It is still random but it is generated within the tests
< MarcoFalke>
as long as the source is reproducible/deterministic, it can be from anywhere
< michaelfolkson>
And by that you mean a reproducible source of randomness is one that supplies the same inputs when it is asked for randomness
< michaelfolkson>
Everytime it is asked for randomness it provides say inputs of 7,2,3 as the first three random inputs
< michaelfolkson>
The problem becomes when you ask it for randomness twice and it provides different inputs on the second time vs the first
< MarcoFalke>
yes, when "s/time/program execution/"
< michaelfolkson>
Gotcha. Thanks!
< michaelfolkson>
How would you explain what a fuzzing "harness" is?
< bitcoin-git>
bitcoin/master 8dbb7de Pieter Wuille: Add comments to VerifyTaprootCommitment
< bitcoin-git>
[bitcoin] MarcoFalke merged pull request #20207: Follow-up extra comments on taproot code and tests (master...202010_taproot-comments) https://github.com/bitcoin/bitcoin/pull/20207
< michaelfolkson>
How does one do that jnewbery? Just manually edit that dev wiki page?
< sdaftuar>
has anyone looked at m_addr_known to see if we should be manually clearing it every day? from a quick look at my logs, i estimate that my addr messages to my peers average substantially less than 5000 messages per day, which i think means that our daily self-announcements would propagate less frequently than we'd expect (maybe once every few days, instead of once per day)
< jnewbery>
michaelfolkson: yes, just edit the page
< jnewbery>
they contain lots of references to further reading for anyone who's curious
< ariard>
you can start with the docs of AFL/libfuzzer/hongfuzz and look for talk of their authors :)
< sipa>
michaelfolkson: what do you mean by p2p fuzzing btw?
< MarcoFalke>
there are already p2p fuzzers
< emzy>
Hi
< michaelfolkson>
Yeah there is P2P code. And P2P experts in this meeting. It would be good eventually to bring that expertise on what should be fuzzed in a P2P context
< michaelfolkson>
(I think)
< MarcoFalke>
I am working on increasing their coverage
< ariard>
IIRC practicalswift added coverage for a part of the p2p stack a while ago ?
< michaelfolkson>
I guess it is an issue of whether fuzz coverage is something the expert fuzzers do or something that everyone thinks about when they open PR etc
< MarcoFalke>
michaelfolkson: The ci does it on every PR
< jnewbery>
I don't run the fuzzers locally because of the extra overhead of building them. I also expect most bugs except the very shallow ones to be discovered by fuzzers running continuously on dedicated machines.
< vasild>
hi
< sipa>
michaelfolkson: i think you need to distintuish between "writing fuzzers" and ",running fuzzers"
< michaelfolkson>
Presumably say the P2P experts should also think about what should be fuzzed in a P2P contexts
< MarcoFalke>
sipa: and "generating seeds"
< michaelfolkson>
sipa: Right, could discuss both
< sipa>
running fuzzers is something everyone can do
< sipa>
writing fuzzers... that's just one way among dozens of how you can increasy confidence in code you're writing
< sipa>
up to the author and what people believe is important in that context
< michaelfolkson>
Should we be thinking about writing fuzzers in the same way as we do writing unit tests and writing functional tests? Everybody should be thinking about adding them? Or just the fuzz test experts/team
< michaelfolkson>
I guess that's my question on the writing them side
< michaelfolkson>
And then on the running them side it is how do we get more people to run them as part of their workflow
< jnewbery>
I don't think there's a problem that needs to be solved here
< nehan>
hi. i found the PR club extremely helpful in learning how to fuzz. i was able to relatively quickly doing it having zero experience with it before.
< sipa>
obbiously?
< MarcoFalke>
michaelfolkson: I guess it is up to the author. Not everyone is adding unit tests or functional tests.
< sipa>
michaelfolkson: again, it's one way among many of increasing confidence incyour code. some things lend themselves well for fuzz testing, others not so much
< michaelfolkson>
MarcoFalke: Not everyone is but everyone should at least be considering whether to or not. Right?
< MarcoFalke>
michaelfolkson: Ideally, yes
< michaelfolkson>
Ok thanks, that's helpful
< jnewbery>
any other topics?
< michaelfolkson>
I think the jnewbery point on what should be done on dedicated machines is something I have also wondered
< sipa>
michaelfolkson: run the fuzzers
< ariard>
a short note on state of tx-pinning/package-relay and discussion on the LN-side?
< michaelfolkson>
And by case of tx-pinning you mean another scenario in Lightning where tx pinning is a problem. Rather than a new way to tx pin in Core
< sipa>
glozow: all sighash modes are effecrively *intentional* malleability, they're there so soke tgings can be changed in a tx without affecting the sig
< ariard>
michaelfolkson: yes we discovered another vulnerability but it wasn't mentinonned on the ML
< glozow>
ahhhh thanks
< ariard>
michaelfolkson: note also that pinning doesn't only affect LN but also bitcoin protocol like vaults, DLC
< nehan>
nit ariard: I don't think you should use the term "split brains" to describe differing mempools. as you point out in the doc, mempools are different *by design*. saying it's a "split brain" implies there should be an unsplit (single) brain.
< nehan>
ariard: "split brain" is usually used to describe a temporary network partition in distributed systems.
< ariard>
nehan: thanks, I think the expression is from zeeman and was just reused at it is by t-bast
< michaelfolkson>
So the messaging is like tx pinning should be even more of a priority to solve in Core than we already knew it was
< sipa>
i don't think it can be solved in general
< ariard>
nehan: I think I used mempool-partition to describe the same phenomena but if you have a better distributed system expression?
< nehan>
ariard: not off the top of my head! will think about it.
< jnewbery>
inconsistent mempools
< ariard>
sipa: define general ? if you mean for any bitcoin protocol without care of protocol designer I likely agree with you
< ariard>
michaelfolkson: note that cdecker proposed a new tx-relay overlay to solve pinning instead of change to p2p tx-relay
< sipa>
ariard: i think dos protection will always result in an inability to accept transactions in some scenarios where that transaction would have been accepted from p2p in another state
< sipa>
we can reduce the set of situations under which that can happen using better algorithms
< sipa>
but the problem in a generic sense seems inherent
< michaelfolkson>
ariard: So that would potentially be run by the Core nodes that care about Lightning? And the Core nodes that don't care wouldn't run it
< ariard>
but for the other something like RBF-package relay _or_ rejecting low-fee pinning transaction might be a solution
< sipa>
ariard: my poijt is you'll always be able to find more things like that
< sipa>
and no solution (that doesn't introduce DOs attacks) is going to leave (possibly complicated) forms of pinning enabled
< sipa>
if it's only speaking forms of pinning that pose problems to you then it'd be good to know what those are
< ariard>
ariard: I don't understand fully your last sentence, if you mean we'll always have complicated cases of pinning I agree?
< sipa>
yes
< sipa>
ok, then we agree :)
< ariard>
okay so we agree on this, what I'm more interested to solve are the easy-to-execute pinning describe as scenario 1) and 2) in my june mail
< michaelfolkson>
All pinning is a problem. There are no specific forms of pinning that are particularly problematic I think...
< ariard>
to mitigate more complicated scenario we had this disccusion with matt about redundant tx-relay broadcast, but that's outside the tx-relay model
< cdecker>
Notice that the alternative transport over the lightning overlay is not intended to be a complete solution, just reduce the effectiveness of pinning by making it less likely to succeed
< ariard>
like some broadcast your transaction over DNS or radio :)
< sipa>
oh
< sipa>
i was hoping LN, because it's an inherently different situation than bitcoin's identityless P2P
< ariard>
like it's easy to pin a LN transaction if you know the full-node, but if this victim has another entry point to the p2p network you won't succeed
< ariard>
sipa: yes, we had this discussion about embedding transaction in LN's onion, it does fit in the standard onion size
< ariard>
more you have redundant tx-relay network, better you're :)
< sipa>
better you're what?
< ariard>
harder it is for an attacker to jam with your tx-relay
< jnewbery>
"the more redundant networks you have, the better off you are"
< ariard>
cdecker: yeah and it was relying on such lighthning overlay being adopted by miners? or what was the state of the discussion :) ?
< sipa>
jnewbery: ah thanks
< ariard>
jnewbery: ah, thanks
< ariard>
anyway I didn't intend to occupy the spot, I'm working on demoing the tx-pinning LN attack we know about and learn from it
< cdecker>
Yeah, I was hoping that it'd be attractive for miners to listen to the overlay since the feerate is higher (even though they don't beat absolute fee, which is really not the miner's main motivation)
< ariard>
that might be a way to align incentives, but still I'm not sure miners are great at maintaining an extended software stack
< ariard>
is anyone wants to do p2p review beg ?
< michaelfolkson>
What do you mean? You want to discuss the beg process?
< michaelfolkson>
Or beg for particular PRs?
< cdecker>
If there's interest from miners to get access I can write up a faux-bitcoin-node that acts as a bridge between bitcoin p2p and the ln overlay relay network
< ariard>
michaelfolkson: I mean p2p meetings are a nice spot for people to ask for reviews on their PR
< michaelfolkson>
Ah ok
< michaelfolkson>
Feel free to beg :)
< jnewbery>
any other topics?
< sipa>
gleb: want to mention the erlay bip updates?
< gleb>
Oh yeah sure.
< jnewbery>
#topic erlay bip updates
< core-meetingbot>
topic: erlay bip updates
< gleb>
So sometimes reconciliation fails initially (first sketch is insufficient, because it's too small and allows to decode up to N differences)
< gleb>
We then don't give up, but make one more attempt.
< gleb>
There are 2 ways to make one more attempt while *still being 100% efficient*: bisection and extension. They are independent alternatives.
< gleb>
We used to do bisection, because it allows to spend a bit less CPU cycles on computing sketches (and just a cool trick I guess)
< gleb>
In practice, the implementation turned out to be too complicated on the Bitcoin Core p2p protocol side.
< gleb>
So I switched the code to do sketch extensions instead. It's much less code, and the code complexity is now more aligned with general Bitcoin project complexity.
< gleb>
And the fact it's a bit more CPU expensive doesn't matter because we expect sketches of very low capacity.
< gleb>
For more rationale see the updated BIP, lemme give you a link.
< jnewbery>
vasild: if you're still around, I have a question about addrv2 support negotiation. There seems to be a discrepency between the bip and the implementation in Bitcoin Core
< jnewbery>
(which may be before the peer has sent verack)
< jnewbery>
I think ideally, it'd be sent between version and verack, like the wtxidrelay message
< vasild>
hmm
< vasild>
they seem to disagree indeed
< sdaftuar>
that reminds me - i noticed that we send "sendaddrv2" messages to block-relay-only peers. is that intentional or desirable?
< vasild>
for the implementation it does not matter when sendaddrv2 is sent and received, but I think there were considerations that no messages should be sent before sending verack (and WTXIDRELAY is somehow an exception to this)
< sipa>
jnewbery: hmm, so really we have in practice two different styles of negotiation (after sending version, and after sending verack), and the bip actually suggests a third (after receiving verack)
< vasild>
and since it is not necessary to send sendaddrv2 before sending verack I chose to send it after (don't add to the exception of WTXIDRELAY)
< jnewbery>
if we tie addrv2 to the same p2p version as wtxidrelay (70016) we can change it to send between version and verack
< vasild>
sdaftuar: it is not intentional
< sipa>
well, for addrv2 it doesn't matter
< jnewbery>
sdaftuar: we could skip sending the message to block-relay-only peers, but I don't think it does any harm to send it
< sipa>
jnewbery, sdaftuar: agree; it seems pointless but harmless
< sdaftuar>
i guess the ship has sailed on adding a flag to that message to indicate whether we participate in addr relay?
< sipa>
yes
< jnewbery>
there are other pointless-but-harmless messages during negotiation. We'll send wtxidrelay and sendaddrv2 to feeler connections before immediately disconnecting
< sipa>
i'm happy we got bip154 in 0.21, but this isn't the time to change it anymore...
< sipa>
*bip155
< sdaftuar>
i'm just trying to figure out what to do with my block-relay-only peer negotiation proposal
< sdaftuar>
previously i had thoguht about requiring that turning on block-relay-only would turn off addr-relay
< vasild>
maybe bip155 should be corrected: "sendaddrv2 SHOULD be sent after receiving the verack message" s/receiving/sending/
< sdaftuar>
but it seemed overly prescriptive in some ways, and addr-relay should perhaps be negotiated separately
< sdaftuar>
but if we
< sdaftuar>
but if we're giving up on addr-relay changes now, then perhaps i'll go back to my initial proposal
< jnewbery>
sdaftuar: is there any benefit in turning off addr relay? We can just ignore any addr messages we get from that peer
< sipa>
vasild: i think that's the simplest fix
< sdaftuar>
i think it would helpful for the peer to know it shouldn't relay addresses to us because we're a black hole
< sdaftuar>
so that it can relay addrs to other peers instead
< jnewbery>
we already send addrs to light clients, which are addr relay black holes, I think
< sdaftuar>
seems a shame that "honest" behavior results in somewhat worse addr propagation
< sdaftuar>
i think that should be fixed or improved as well
< vasild>
sdaftuar: I think "I participate in addr relay" and "I maintain a usable/complete addr database" should be negotiated/announced separately from sendaddrv2 (I support addrv2 format), even if we could change sendaddrv2 now.
< sdaftuar>
vasild: worth adding a new p2p message for it?
< sdaftuar>
we could go that route as well
< sdaftuar>
i just was afraid that adding more p2p messages seems to raise complaints about the protocol being confusing or redundant
< sdaftuar>
though it's free
< jnewbery>
vasild: your suggested change to BIP155 isn't what Bitcoin Core is doing. It sends the message after receiving a version message
< sipa>
jnewbery: and after sending the verack, no?
< jnewbery>
sipa: ah yes, you're right.
< sipa>
sdaftuar: how do you suggest dealing with spv clients being black holes?
< jnewbery>
do we really want to allow peers to switch from addrv1 to addrv2 at any point?
< vasild>
sdaftuar: what about introducing one generic message for announcing capabilities, like "I can do X, Y, Z". That idea was shot last time I mentioned it, IIRC...
< sipa>
jnewbery: we already do, i think
< sdaftuar>
jnewbery: is there any problem with peers switching from addrv1 to addrv2?
< sipa>
jnewbery: addrv2 negotiation is really simple, it's just a message indicating "you're as of now allowed to send me addrv2 instead of addrv1"
< sdaftuar>
sipa: my first thought was that if we allowed peers to opt-in (or out) of addr-relay, then we should just treat spv clients as honest network participants
< sipa>
sdaftuar: which means?
< sdaftuar>
if they want to participate in addr-relay, they can; the problem becomes one of software being broken for not implementing a negotiation feature if they don't relay addresses but indicate that they do
< sdaftuar>
which we can't control anyway
< sdaftuar>
the problem in my view is that honest software has no way to communicate not to send addr messages because they will get dropped
< sipa>
sdaftuar: i guess my question is: with this new addr relay negotiation... do you make it "addr relay is by default on for NODE_NETWORK(_LIMITED) peers, off otherwise"
< sipa>
or something like that, so that old spv software is set off by default
< jnewbery>
It just seems easier to think about if we don't need to worry about addrv2 support changing during a peer's lifetime. I've had a quick skim of the code and agree that it's simple, but still think that the ability to effectively renegotiate addrv2 support is an unnecessary complexity.
< sipa>
jnewbery: i agree, but i also think it's a cost we've already paid effectively
< sdaftuar>
jnewbery: it only goes in one direction right?
< sipa>
it works
< jnewbery>
put another way: if we weren't currently able to renegotiate from addrv1 to addrv2 during a peer's lifetime, and I proposed that we added that as a feature, I think the suggestion would be NACKed immediately
< sipa>
yes
< sdaftuar>
jnewbery: only because unnecessary changes are, well, unnecessary :)
< sipa>
but the same may be true in the other direction :)
< sipa>
making it impossible to renegotiate would need more code, not less
< jnewbery>
sipa: code complexity isn't a one-off cost. It's an ongoing burden for all current and future contributors
< sipa>
jnewbery: of course
< jnewbery>
sipa: no it wouldn't. The spec would say: "send sendaddrv2 between version and verack". Receiving the message at any other time would be ignored
< sipa>
jnewbery: and right now, it doesn't need code ro decide whether to ignore it or not
< sipa>
it's all trivial changes of course, but i don't think it's clear cut that one alternative is obviously simpler than the other
< jnewbery>
sipa: yes, the burden is placed on the developer to audit whether it's safe to make that transition during the peer's lifetime, instead of it just being const
< sipa>
you might have a point if *all* negotiation were between version and verack, but that's already not the case and won't be
< jnewbery>
and the burden is also placed on any future developer who makes changes to the way addr relay works, to be aware that addrv2 support can change at any point
< sdaftuar>
sipa: i think defaulting addr relay to off for non-NODE_NETWORK(_LIMITED) peers would be fine. (or eventually make that the default to give spv client authors sufficient warning, inc ase it's a surprise)
< sipa>
jnewbery: i agree that's something to keep in mind... but is that really worth changing the code (and spec) for, given that there are several other things that behave in exactly the same way already?
< sipa>
as opposed to say, add a code comment to clarify addrv2 status can change af any point
< sipa>
(and sendheaders, and filter status, and ...)
< vasild>
"send sendaddrv2 between version and verack" --> "send sendaddrv2 between sending version and sending verack" or "send sendaddrv2 between receiving version and receiving verack"
< jnewbery>
sipa: one or both have to change anyway, and we now know the right way to negotiate features, so if there's another rc, I think we should take advantage of that
< sipa>
i strongly disagree
< sipa>
there is no bug here
< sipa>
the time for something like this has passed
< jnewbery>
vasild: "send addrv2 between sending version and sending verack"
< jnewbery>
there's either a bug in the spec or the implementation
< sipa>
hmm, right
< sipa>
that's fair
< sipa>
i was treating it as a typo in the spec, and assuked it was intended to mimick the existing feature negotiations
< sipa>
and i think because of that, the spec makes no sense
< sipa>
but you could see it differently
< vasild>
to me it looks like a bug/typo in the spec and should s/receiving/sending/
< sipa>
yeah, exactly
< jnewbery>
I don't think it's worth making an rc for this
< vasild>
agree
< sipa>
i think the simplest solution is fixing the spec
< jnewbery>
I think my preferred change to the spec would be to s/after receiving the verack/after receiving the version/. That means that at least we can send our message between version and verack.
< sipa>
(not because it mismatches the implementation, but because it appears unintentional)
< sipa>
does the current 0.21rc code work correctly if it receives addrv2 before verack?
< jnewbery>
Oh, no we can't because we'll only process a sendaddrv2 after receiving a verack.
< jnewbery>
sipa: snap
< vasild>
"does the current 0.21rc code work correctly if it receives addrv2 before verack?" -- yes
< jnewbery>
vasild: no, we'll drop the message if we receive it before verack
< vasild>
sipa: yes, but did not dive in it yet in order to comment
< sipa>
sure, no rush
< vasild>
I did not get it why did you close the other small pr
< sipa>
vasild: it was intended as a quick fix for 0.21
< vasild>
it would get torv3 anchors
< sipa>
not as-is, it needed your patch or 20516 as well
< vasild>
yes, + the patch I posted in the comments
< vasild>
would the small pr + my patch in the comments cause any trouble for 20516? I think no
< sipa>
201516 does exactly the same thing
< sipa>
just cleaner
< sipa>
(imo)
< sipa>
but it's not something i feel is worth it for 0.21 anymore
< sipa>
so i'd rather do it properly instead in 0.21.1 or 22
< vasild>
ok
< sipa>
20516 makes it also fail deserialization if the on-disk version is unexpected, and adds more tests
< sipa>
(i wrote that before you commemted with your patch, otherwise i'd have based it on yours)
< vasild>
aha, since you mention "fail deserialization"...
< vasild>
should we compare the stream version with ondisk version in CAddress deserialization
< vasild>
and if one has ADDRV2_FORMAT set but the other does not then throw?
< vasild>
is this what you mean by "unexpected"?
< sipa>
that'll break anchors
< sipa>
once v2 is used for those, but an old anchors.dar is read
< vasild>
oh, right
< sipa>
the semantics in 20516 is that the ADDRV2_FORMAT in the stream version enables/disables the use of addrv2 in the disk version
< sipa>
so you get an error if it is set in the disk version, but not in the stream version, on deserialize
< vasild>
during serialization?
< sipa>
but the other direction works
< vasild>
I see
< sipa>
i have a follow-up (the serializarion parameter stuff) that just splits all the different serialization types out, and removes ADDRV2_FORMAT from stream versions (but it remains inside the CAddress disk serializer)
< sipa>
unfortunately, that means at least 4 versions
< sipa>
but probably 5; so you have DISK_V1 and DISK_V1_OR_V2 (where pre-v3 addrman would set DISK_V1 which causes an error if a v2 addr on disk is in it)
< vasild>
:-|
< sipa>
hmm, maybe we should just remove the "notime" functionality from CAddress
< sipa>
and just invoke it as serializing nServices and CService spearately
< bitcoin-git>
[bitcoin] sipa opened pull request #20541: Move special CAddress-without-nTime logic to net_processing (master...202012_addr_without_time_is_no_addr) https://github.com/bitcoin/bitcoin/pull/20541
< sipa>
vasild: ^
< promag>
quick question, does it makes sense to limit cookie auth to loopback? anyone using it in other ways?