Remove is{Empty,Full} flags from CBloomFilter, clarify CVE fix (p2p)

https://github.com/bitcoin/bitcoin/pulls/18806

Host: jnewbery  -  PR author: theStack

The PR branch HEAD was 1ad8ea2b at the time of this review club meeting.

Notes

  • A bloom filter is a probabilistic data structure. It supports two operations:
    • adding an element to the filter.
    • querying an element from the filter.
  • If an element has been previously added, then querying for the element will return true. If an element has not been added, then querying for the element may return true or false. In other words, querying may return a false positive, but will never return a false negative.

  • See the wikipedia page for how a bloom filter is implemented with hash functions onto a bitfield. Note that the false positive rate depends on the size of the filter and the number of hash functions.

  • BIP 37 introduced a new method for Simple Payment Verification (SPV) clients to use bloom filters to track transactions that affect their addresses. BIP 37 was implemented in Bitcoin Core in PR 1795.

  • Using the P2P messages defined in BIP 37, an SPV client can request that a full node send it transactions which match a bloom filter. The full node will then relay unconfirmed transactions that match the filter, and the client can request merkle blocks, which only contain the transactions that match the filter.

  • The SPV client chooses the bloom filter parameters (filter size, number of hashes and a ‘tweak’ for the hashes) and sends them to the node in a filterload message.

  • The original implementation contained a logic bug. If the client sent a filterload message with a zero-sized filter, then the serving node could later attempt a divide-by-zero and crash when querying an element from the filter. See CVE-2013-5700 for further details.

  • This bug was quietly fixed in PR 2914 without advertising the reason. That fix added the isFull and isEmpty booleans, which have proven to be confusing for developers.

  • This PR 18806 removes those isFull and isEmpty booleans and adds a more straightforward fix for the issue.

Questions

  1. Did you review the PR? Concept ACK, approach ACK, tested ACK, or NACK? (Don’t forget to put your PR review on GitHub.)

  2. Why was the motivation for PR 2914 not made explicit?

  3. Serving bloom filters is now disabled by default for full nodes. Why? What are some of the problems with BIP 37? What alternatives are there?

  4. How is this PR tested? Are there any additional tests that could be added?

Meeting Log

  113:00 <jnewbery> #startmeeting
  213:00 <achow101> sipa: yes. I was just testing bad versions for the expected error
  313:00 <jnewbery> hi folks. Welcome to the Bitcoin Core PR Review club meeting!
  413:00 <b10c> hi
  513:00 <jnewbery> feel free to say hi to let people know you're here
  613:00 <bordalix> hi
  713:00 <fjahr> hi
  813:00 <pinheadmz> hi! 👋
  913:00 <lightlike> hi
 1013:00 <ccdle12> hi
 1113:00 <raj_149> hi
 1213:00 <theStack> hi
 1313:00 <ariard> hi
 1413:00 <nehan> hi
 1513:00 <thomasb06> hi
 1613:00 <jnewbery> Notes and questions are in the normal place: https://bitcoincore.reviews/18806.html
 1713:01 <jonatack> hi
 1813:01 <vasild> hi
 1913:01 <jkczyz> hi
 2013:01 <the_nomad> Hello chaps
 2113:01 <michaelfolkson> Hi
 2213:01 <emzy> Hi
 2313:01 <willcl_ark> hi
 2413:01 <r251d> hi
 2513:01 <handed> hi
 2613:01 <brikk> hi
 2713:01 <nehan> the_nomad: we are not all chaps :)
 2813:01 <sipa> hi
 2913:02 <jnewbery> This week we're covering quite a straightforward refactor PR. There have been a lot of new participants at these meetings the last few weeks, and recent meetings have gone quite deep into P2P and validation logic, so I though it'd be nice to look at something a bit simpler this week.
 3013:02 <ariard> wait isn't since 1-year bitcoin pr review club has been started ?
 3113:02 <andrewtoth> hi
 3213:02 <nehan> happy anniversary jnewbery!
 3313:02 <jnewbery> it's also a great opportunity to ask more basic questions about the Bitcoin Core review process
 3413:03 <pinheadmz> !! !! !! 1 whole year !! !! !!
 3513:03 <fjahr> happy anniversary \o/
 3613:03 <theStack> happy anniversary also from my side!
 3713:03 <sipa> congrats jnewbery and all who help running this, keep going :)
 3813:03 <b10c> ariard: just checked. I think you are right! Happy anniversary!
 3913:03 <raj_149> Happy anniversary. :D
 4013:03 <jnewbery> happy anniversary everyone! I only host about once a month now. This club belongs to all of us!
 4113:03 <ariard> jnewbery: hey happy anniversary, a litte gift from people here https://gist.github.com/ariard/6a40fe158d419126a8e5b0f36d691f28 :)
 4213:04 <pinheadmz> woo hoo!!! HBD club <3
 4313:04 <jnewbery> awww thanks everyone!
 4413:04 <ariard> and they ots proof for onchain commitment ahah https://gist.github.com/ariard/b6014d1a887182e0c7cdd37589ec339e ;)
 4513:04 <amiti> :D
 4613:04 <ariard> *the
 4713:04 <willcl_ark> handsup.gif
 4813:04 <lightlike> happy anniversary!
 4913:04 <andrewtoth> happy anniversary! thanks jnewbery!
 5013:05 <pinheadmz> no-one's crying! (sniff)
 5113:05 <sipa> i hope everyone remains committed and committed on-chain to keep doing this :p
 5213:05 * sipa hides
 5313:05 <ariard> you can do `ots verify digital_card.ots`
 5413:05 <jnewbery> I'm not crying, honest
 5513:06 <pinheadmz> ariard: great job :-) ty
 5613:06 <jnewbery> that's really touching. Thanks everyone!
 5713:06 <raj_149> Thank for the amazing work you do jnewbery. I have learned a lot from this club.
 5813:06 <jnewbery> ok, let's talk about Bitcoin
 5913:07 <ariard> yeah always talk about bitcoin
 6013:07 <theStack> :)
 6113:07 <sipa> hey did you guys know the blockly subsidy halves next week!?
 6213:07 <thomasb06> I have a basic question...
 6313:08 <pinheadmz> sipa: 761 more blocks !
 6413:08 <jnewbery> The PR was merged today, but it's a good idea to still review merged PRs. https://github.com/bitcoin/bitcoin/pull/18806
 6513:08 <sipa> guys and non-guys
 6613:09 <willcl_ark> s/you guys/y'all ;)
 6713:09 <jnewbery> who had a chance to review the pr? y/n
 6813:09 <brikk> y
 6913:09 <fjahr> y
 7013:09 <raj_149> y
 7113:09 <pinheadmz> y
 7213:09 <the_nomad> y
 7313:09 <nehan> y
 7413:09 <ariard> y
 7513:09 <lightlike> y
 7613:09 <theStack> sipa: oh yes -- unfortunately there won't be any possibility to physically participate in a halving party :/
 7713:09 <vasild> y
 7813:09 <bordalix> y
 7913:09 <andrewtoth> y
 8013:09 <jnewbery> thomasb06: feel free to ask questions at any point. No need to wait
 8113:09 <michaelfolkson> y
 8213:09 <r251d> y
 8313:09 <thomasb06> Is it possible to review a PR without being able to code it?
 8413:09 <ccdle12> y
 8513:09 <jnewbery> Wow. Lots of review. No wonder it got merged
 8613:10 <jonatack> happy 1st birthday to the review club everyone 🏆 🚀
 8713:10 <sipa> y
 8813:10 <jkczyz> y
 8913:10 <jnewbery> thomasb06: yes! You can still test the code and leave a comment saying what you did
 9013:10 <bordalix> hummmm, I review it but didn't ACK on the repo. Do that count?
 9113:10 <fjahr> much more y's than reviews in the PR, I think more people should post their reviews on GH :)
 9213:10 <thomasb06> jnewbery: cool... This was my main concern
 9313:10 <jonatack> fjahr: +1
 9413:11 <jnewbery> don't expect to fully understand everything on your first reviews. As you get more experience, you'll understand more and more
 9513:11 <michaelfolkson> thomasb06: Concept ACK doesn't need code review. Nor does running tests
 9613:11 <jonatack> thomasb06: i do hope so... i do it all the time :p
 9713:12 <jnewbery> fjahr: definitely. If you're reviewing but not leaving comments, then you're depriving everyone else of your knkowledge and insights
 9813:12 <brikk> thomasb06: I think a good start is to do what I did, checkout the branch, figure out how to build the code and run the tests
 9913:12 <jnewbery> ok, first question: Why was the motivation for PR 2914 not made explicit?
10013:12 <thomasb06> but... michaelfolkson ok, you answered the question before I asked
10113:12 <vasild> because it was a security vulnerability
10213:12 <pinheadmz> jnewbery: an attacker could remotely crash bitcoind by setting a bloom filter of 0 size
10313:12 <handed> jnewbery flaw could bring down nodes by exhausting resources
10413:12 <raj_149> because it was a bug.
10513:13 <nehan> jnewbery: code is pushed before it's deployed to the network, so someone would have seen the possible attack
10613:13 <thomasb06> brikk: ok, thanks
10713:13 <jnewbery> vasild pinheadmz nehan: yes!
10813:13 <lightlike> i wonder if a covert fix like this would still be possible today - someone surely would challenge the claim that this is a performance optimization and ask for benchmarks?!
10913:13 <jnewbery> handed: almost. It could bring down a node, but it's by a segfault, not resource exhaustion
11013:13 <michaelfolkson> lightlike: You would hope so ;)
11113:14 <sipa> not a segfault, a division by zero
11213:14 <theStack> lightlike: interesting question, i also asked myself this quite a lot... i think the quality of reviews has increased drastically, looking at old PRs
11313:14 <fjahr> lightlike: maybe the covert techniques have improved as well ;)
11413:14 <jnewbery> raj_149: we fix lots of bugs overtly all the time. This was covert because of the nature and seriousness of the bug
11513:14 <jnewbery> sipa: yup. Thanks
11613:14 <handed> sipa isn't burning up the CPU resource exhaustion or am I not translating accurately?
11713:14 <raj_149> jnewbery: The first comment of gmaxwell in 2914 gives away the motivation. So how its still a covert fix?
11813:14 <vasild> lightlike: so one would also have to do some real improvement in performance + secret fix merged in one patch :-D
11913:14 <michaelfolkson> So this could have knocked all v0.8 nodes off the network?
12013:15 <sipa> handed: not sure what you mean by burning up the CPU; the node would just crash with a division by zero trap
12113:15 <sipa> instantly, without any cpu or memory usage
12213:15 <sipa> michaelfolkson: all reachable ones, yes
12313:15 <handed> ok, I thought it would cause some excess computation which would cause the crash, ty for clarifying
12413:15 <jnewbery> raj_149: it was claimed that the problem was resource wastage. The actual problem was instacrash
12513:16 <handed> jnewbery ty for clarification
12613:16 <raj_149> oh i see..
12713:16 <ariard> if people are interested by cover fix, see https://github.com/bitcoin/bitcoin/pull/11397 and https://lists.linuxfoundation.org/pipermail/bitcoin-dev/2019-November/017453.html
12813:16 <ariard> *covert
12913:17 <jnewbery> michaelfolkson: I believe bloom filters were on by default, so I think it would be able to knock out the majority of nodes at the time
13013:17 <sipa> jnewbery: i don't think it was possible to turn them off
13113:17 <michaelfolkson> jnewbery: Majority of 0.8 nodes? No bloom filters pre 0.8
13213:18 <jnewbery> sipa: oh interesting. Yes, I see that's explicit in BIP 111: https://github.com/bitcoin/bips/blob/master/bip-0111.mediawiki
13313:18 <jonatack> ariard: thanks
13413:18 <jnewbery> "BIP 37 did not specify a service bit for the bloom filter service, thus implicitly assuming that all nodes that serve peers data support it."
13513:19 <jnewbery> ok, next question: Serving bloom filters is now disabled by default for full nodes. Why? What are some of the problems with BIP 37? What alternatives are there?
13613:19 <michaelfolkson> There were 3 CVEs in that 0.8 release...
13713:19 <theStack> by the way, pr #2914 was buggy as well and actually didn't solve the problem (a followup PR by gmaxwell followed soon after though :))
13813:20 <lightlike> theStack: interesting, what was the id of the follow up?
13913:20 <pinheadmz> jnewbery: server side filtering means more work for publicly acesible nodes, and neutrino offers a better privacy and security model
14013:20 <michaelfolkson> Problems with BIP 37. Privacy, false positives
14113:20 <theStack> lightlike: #2919
14213:21 <handed> bip 37 enables amplification attacks by clients to servers of the filters
14313:21 <pinheadmz> handed: how is that? whats amplified?
14413:21 <jnewbery> pinheadmz michaelfolkson: yes, problems with BIP 37 are server resource usage and poor privacy
14513:21 <handed> you can spam many requests to a server at very little cost and cause a disproportionaitly high amount of compute load
14613:22 <andrewtoth> an alternative is compact block filters in general, neutrino is a specific implementation using them to sync I believe
14713:22 <ariard> there was a a risk of disk I/O Dos, see a Poc attack https://github.com/petertodd/bloom-io-attack
14813:23 <jnewbery> andrewtoth: thanks for that clarification. Neutrino is an implementation of BIP 157/158
14913:24 <jnewbery> We covered the proposed BIP 157 implementation in a previous review club: https://bitcoincore.reviews/16442.html
15013:25 <theStack> one of the major advantages of server side filters is that they are deterministic, i.e. no need to maintain an individual state for each peer?
15113:25 <jnewbery> Any other questions about BIP 37 or should we move on?
15213:25 <jnewbery> theStack: exactly. It means they can be cached and delivered from anywhere, and a client can fetch from diverse sources
15313:25 <ariard> theStack: yeah it's O(1) in CPU computation whatever the number of clients served
15413:25 <handed> theStack +1
15513:26 <sipa> theStack: no cpu cost per request, ability for client to compare filters from multiple nodes, ... possibility even to at some point soft fork it in so that you have an SPV-level proof of no censorship, ...
15613:27 <jnewbery> ok. Final question (but feel free to ask anything about BIP 37 / BIP 157/158 if you think of it later)
15713:27 <jnewbery> How is this PR tested? Are there any additional tests that could be added?
15813:28 <pinheadmz> there was a test in https://github.com/bitcoin/bitcoin/pull/18515
15913:29 <ccdle12> functional test that sends an empty nElements
16013:29 <raj_149> it was tested by the p2p_filter.py functional test.
16113:29 <jnewbery> pinheadmz: right, and https://github.com/bitcoin/bitcoin/pull/18672
16213:30 <jnewbery> Nice approach to add a test first, and then show that the refactor doesn't change behaviour
16313:30 <michaelfolkson> And a fuzz test
16413:31 <andrewtoth> didn't we review the fuzz test in here a while back?
16513:31 <jnewbery> ok, that was all the questions I had. I wanted this week's meeting to be a chance for people to ask more general questions about the review process, so hopefully some of you have questions.
16613:31 <jonatack> it's in the PR description -> 18521
16713:31 <theStack> ad server side filters: seems to be a great idea and actually seems much more logical than client side filters; one drawback though seems to be that more data has to be transferred
16813:31 <jnewbery> Don't be shy if it's your first time!
16913:32 <r251d> Sorry if I missed the time to ask this question, but could an empty `vData` cause a problematic modulo division by zero here? https://github.com/theStack/bitcoin/blob/1ad8ea2b73134bdd8d6b50704a019d47ad2191d8/src/bloom.cpp#L43
17013:32 <theStack> jnewbery: it was not planned from me originally to refactor -- that was inspired by a recent pr review club meeting on fuzzing :)
17113:32 <jnewbery> andrewtoth: we reviewed a different fuzz test: https://bitcoincore.reviews/17860.html
17213:32 <sipa> r251d: that's exactly where the bug was triggered
17313:32 <brikk> r251d: exactly what I was going to ask too :)
17413:33 <sipa> r251d: and it's avoided by making sure that function can't be called when vData is empty
17513:33 <lightlike> andrewtoth: there is a specific one src/test/fuzz/bloom_filter.cpp - we reviewed one on process_messages that indirectly also creates the msgs for the bloom if you let it run for a while
17613:33 <raj_149> r251d: isn't thats what exactly the bug was?
17713:33 <brikk> why is there no vData.empty() check in this method?
17813:33 <theStack> also i was very unsure if this would get concept acks, as bip37 is kind of anachronistic and according to gmaxwell there were discussions about removing bloom filter support completely years ago
17913:33 <brikk> doesn't that add a risk of another method calling it without first checking if it's empty?
18013:33 <nehan> brikk: i was wondering the same thing. why not check closer to use?
18113:33 <theStack> happy that i could help and that it was accepted though
18213:33 <sipa> brikk: it's already too late by that point
18313:34 <r251d> I see. The calling functoins all check vData.empty() and return before calling Hash()
18413:34 <sipa> there is no reasonable answer the function can give
18513:34 <jnewbery> theStack. I'm glad you did. Cleaning up code so it clearly matches expectations is a worthy exercise!
18613:34 <sipa> it could contain an assert
18713:34 * vasild afk
18813:34 <nehan> or at least a comment that says that the caller of this function is responsible for making sure vData.size() is not 0
18913:35 <sipa> yeah that would make a lot of sense
19013:35 <jkczyz> jnewbery: Regararding testing, a unit test where CBloomFilter is constructed with nElements == 0 would also catch this
19113:35 <jnewbery> jkczyz: I agree. A unit test seems like a natural place to test this.
19213:36 <ccdle12> I was just wondering if CBloomFilter construction in net_processing by "vRecv >> filter" is constructed via `serialize.h` and is using those macros the preferred way to deserialize to objects in Bitcoin?
19313:36 <jnewbery> I found it slightly weird that CBloomFilter has two ctors, one that is used in the product code and one that is only used in tests
19413:37 <theStack> jnewbery: agreed, this ctor also confused me quite a lot when i started digging into this code... a comment like "only used for tests" would make sense here
19513:37 <sipa> ccdle12: filter is constructed at the time it is defined; deserialization only fills it
19613:38 <michaelfolkson> Let's say an attacker had successfully pushed all v0.8 nodes off the network. Is there anything he/she could've done other than causing havoc? Target a specfic node with sybils when it comes back up?
19713:38 <nehan> question on reviewing: this was already merged. is there any benefit in leaving a comment saying it would be nice to have an assert in Hash()?
19813:38 <jnewbery> net_processing only uses the constructor with no parameters
19913:38 <lightlike> but would an assert help? does it matter much if the node crashes b/c of a divide by zero or bc the assert is triggered?
20013:38 <sipa> lightlike: no... it might make debugging slightly easier, but that's it
20113:38 <theStack> nehan: i think yes, as someone could grasp an idea for a follow-up pr
20213:39 <nehan> fair point!
20313:39 <handed> nehan: what's the harm, I think it's positive for people reviewing the reviews to know how people felt about the PR
20413:39 <jnewbery> nehan: I think so. Even better would be to offer to open that PR
20513:39 <sipa> lightlike: the main advantage of an assert is documenting the expectations
20613:39 <thomasb06> Are there PRs for tests too, I'd rather review Python than C++?
20713:39 <ccdle12> sipa: I see thanks that makes sense, so I guess the msg type is already validated before attempting deserialize?
20813:39 <jonatack> nehan: i think post-merge review is great, either to prep a follow-up or even better, if you catch a regression!
20913:39 <sipa> thomasb06: many!
21013:39 <thomasb06> sipa: cool
21113:40 <sipa> ccdle12: eh, i think you're confused
21213:40 <theStack> lightlike: well at least one would know _where_ it crashed :)
21313:40 <jnewbery> thomasb06: yes, there are lots. Look for PRs that have the Tests label: https://github.com/bitcoin/bitcoin/pulls?q=is%3Aopen+is%3Apr+label%3ATests
21413:40 <sipa> ccdle12: deserialization can definitely fail if the data in the stream is not a valid CBloomFilter
21513:40 <sipa> in which case it will throw an exception
21613:40 <handed> michaelfolkson re: attacker, if the node that submits block headers to a pool is crashed then that could create a lower effective hashrate / hmm maybe a chain split?
21713:41 <thomasb06> jnewbery: thanks, that's where I need to start
21813:41 <ccdle12> sipa: aah ok thanks, that's what I was wondering, looks like I'll need to dive into the serialize h file :)
21913:41 <jnewbery> There were a couple on twitter from @nickycutesc
22013:41 <jnewbery> *couple of questions
22113:42 <jnewbery> "What’s the best way to debug unit tests?
22213:42 <michaelfolkson> Twitter? What is that? :)
22313:42 <jnewbery> and "Favorite IDE? gdb on a terminal is....interesting."
22413:43 <jonatack> fjahr: can you repost the new home for your debugging gist?
22513:43 <sipa> EDLIN.COM
22613:43 <thomasb06> jnewbery: what does a review consist in, read the modifications brough and if agreed acknowledge with a Concept ACK?
22713:43 <jnewbery> https://github.com/fjahr/debugging_bitcoin
22813:43 <michaelfolkson> handed: I think you'd need to sybil the pool. Any honest connection and the chain wouldn't split?
22913:43 <fjahr> https://github.com/fjahr/debugging_bitcoin :) still need to put some more work in but should be helpful nevertheless
23013:44 <jnewbery> fjahr: that's a great resource. Thanks for putting it together
23113:44 <nehan> fjahr: i would like you to add equivalent linux commands :)
23213:44 <raj_149> jnewbery: i have managed to debug unit tests in vscode. gdb felt little intimidating.
23313:44 <nehan> fjahr: i'm happy to help if you would like it
23413:44 <sipa> thomasb06: https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md
23513:45 <thomasb06> sipa: thanks
23613:45 <jnewbery> thomasb06: specifically the section on Decision Making Process
23713:45 <jonatack> thomasb06: have a deep look at the articles mentioned on the review club home page, and spend time watching interation on github and looking at the docs and code in the repository
23813:45 <emzy> sipa: the DOS program?
23913:45 <fjahr> nehan: sure, it's a repo now instead of a gist so people can contribute :)
24013:45 <michaelfolkson> https://jonatack.github.io/articles/how-to-review-pull-requests-in-bitcoin-core
24113:45 <lightlike> i usually just add printfs for debugging
24213:46 <handed> michaelfolkson I'm thinking that if mining operator doesn't get a new block header from pool operator (e.g. they continue mining on old headers because their pool operator's node has crashed due to CVE) that they can create another valid block B2, A<-B1 A<-B2
24313:46 <jnewbery> printf/vim for me, but I don't want to start any religious wars
24413:46 <handed> i.e. they're building work on an outdated tip
24513:46 <ariard> thomasb06: try to understand the code structure and how any changes may alter program behavior, and if this behavior is expected or not compare to what his claim by author
24613:47 <ariard> and how can you make with ways to assert code is correct, by adding functional tests, manual testing, change few parameters like new timers
24713:47 <thomasb06> ariard: ok
24813:47 <ariard> also you can look about previous changes on the same code area, what kind of issues is raised generally
24913:48 <ariard> like if you do look on p2p, you care a lot about DoS vectors
25013:48 <michaelfolkson> handed: Ok so that impacts the miners profits but whenever they get back online that doesn't impact the rest of the network
25113:48 <michaelfolkson> Those blocks they "mined" get dropped like any normal chain re-org when they come back up
25213:49 <handed> michaelfolkson it delays settlement for the network, seems to me that uncrashed nodes will have to wait and say "Hmm two valid, competing blocks exist. I don't know which one miners will continue to build on, I'll need to wait"
25313:49 <sipa> handed: they will prefer the one they saw first
25413:49 <nehan> thomasb06: i try to convince myself the code is correct. also think about how the change might impact performance. more/fewer disk reads? memory conserved?
25513:49 <jonatack> another interesting debugging resource: some of achow101's twitch coding videos... the best parts imo are when he gets into the weeds, stuff is broken, and he has to fix it.
25613:50 <thomasb06> nehan: yep
25713:50 <jonatack> https://www.twitch.tv/achow101
25813:51 <theStack> jonatack: thanks for the link. actually an interesting idea to watch someone coding
25913:51 <nehan> jonatack: i haven't seen that, thanks!
26013:51 <jnewbery> When I started reviewing Bitcoin Core PRs, I did a _lot_ of manual testing. I wanted to see for myself how the behaviour changed. Even if there are automated tests, it's a good exercise to try to trigger the new behaviour yourself
26113:51 <handed> sipa sure but as you point on occasionally, it's all about which block gets built on, not if your tx is in the tip (unless the point is, if the chain diverges under non adversarial txs, all txns could eventually be included)
26213:51 <sipa> handed: that's why you wait for 6 confirmations :)
26313:51 <sipa> (or decide not doing so is an acceptable compromise)
26413:52 <handed> exactly! but do you agree that having two competing blocks decreasing certainty that "6 confirms" have occured?
26513:52 <jonatack> nehan: theStack: for C and c-lightning videos, there is also Lisa Neigut: https://www.twitch.tv/niftynei/videos
26613:52 <handed> maybe I'm being pedantic
26713:52 <fjahr> thomasb06: I gave a talk on the functional tests (the ones in python) but it seems it's still not uploaded yet by Bitcoin Edge. At least the slides are there, maybe they are a little bit helpful https://telaviv2019.bitcoinedge.org/presentations
26813:52 <sipa> handed: sure, but at the same time you generally won't know there is a competing chain
26913:52 <jnewbery> and if a PR didn't have an automated test, sometimes I'd write one and offer it to the PR author
27013:52 <michaelfolkson> sipa handed: I'm thinking havoc is only attacker goal unless they can control the connections to that node when it comes back up
27113:54 <michaelfolkson> sipa handed: Granted we want to stop havoc from happening. But it won't damage network once bug is addressed or cause people to lose money
27213:54 <handed> >"won't know there is a competing chain" I'm rusty, does a node not relay "B2" to peers if it's seen "B1" first?
27313:54 <theStack> jonatack: awesome. i wanted to take a deeper look into lightning and in particular one of its implementations for a long time, maybe this is a good start
27413:54 <sipa> handed: not sure what B1 and B2 are, but nodes will generally only relay their best chain
27513:55 <jnewbery> handed: a node won't relay a block that isn't on its mainchain
27613:55 <handed> A<-B1 A<-B2
27713:55 <sipa> handed: in that case, no they won't
27813:55 <jnewbery> ok, 5 minutes left. Any final questions?
27913:55 <thomasb06> fjahr: great. They are a lot helpful
28013:55 <sipa> handed: it's both a fingerprinting attack, and nodes have an incentive to make the network converge towards their view of the chain
28113:56 <jnewbery> ok, in that case let's wrap it up there. One last thing before we go...
28213:57 <jnewbery> I'm hoping to make some progress on getting jimpo's BIP 157 implementation merged: https://github.com/bitcoin/bitcoin/pull/18876
28313:57 <handed> yeah, is your fingerprinting point: if peers can feed info to nodes and probe out information, they can gain visibility in peer topology?
28413:58 <sipa> handed: yes, exactly
28513:58 * handed thumbs up
28613:58 <jnewbery> I've opened the first PR here: https://github.com/bitcoin/bitcoin/pull/18877. Would anyone be interested in an extra review club session on that? It'd be slightly different from the normal session because it's explicitly to try to help get the PRs into a state ready for review.
28713:59 <michaelf_> Definitely. When?
28813:59 <jnewbery> *ready for merge
28913:59 <the_nomad> Y
29013:59 <fjahr> jnewbery: yes
29113:59 <nehan> jnewbery: yes
29213:59 <theStack> jnewbery: yes
29313:59 <jonatack> jnewbery: wdym by state ready for review?
29413:59 <ccdle12> jnewbery: yes
29513:59 <brikk> nehan: I added a comment about the assert on the PR
29613:59 <jonatack> (RFM)
29713:59 <jnewbery> jonatack: sorry, ready for merge
29813:59 <jkczyz> jnewbery: I plan on reviewing the 18877 today
29913:59 <nehan> brikk: great!
30014:00 <jnewbery> ok, same time tomorrow? We'll continue doing the normal review clubs on Wednesdays
30114:00 <jonatack> jnewbery: at any rate, yes
30214:00 <jnewbery> and on Thursdays try and get somewhere with BIP 157!
30314:00 <jonatack> my main concern is that BIP157 remain opt-in
30414:01 <jnewbery> ok, I've gotta dash. Thanks again everyone. And thanks to ariard for the card. That was very kind.
30514:01 <jnewbery> jonatack: yep, it's off-by-default
30614:01 <jnewbery> #endmeeting