The PR branch HEAD was 4376797 at the time of this review club meeting.
This is the third PR in our special series on the BIP
157
implementation. See the previous meetings on PR 18877 and PR
18960.
Notes
This PR adds support for the getcfheaders P2P message. You should familiarise
yourself with the format of that message and the cfheaders response before
reviewing the code changes. Both of those messages are defined in BIP
157.
The getcfheaders message contains two fields: StartHeight (the height of
the first block in the requested range) and StopHash (the hash of the last
block in the requested range). The message handling code starts by deserializing
those fields into uint_32t and uint256 local variables, respectively.
The PrepareBlockFilterRequest() helper function has its interface changed
to accept start_height and a max_height_diff parameters.
The ProcessGetCFHeaders() function calls a new method on the block filter
indexer class called BlockFilterTypeName().
New tests are added to the p2p_blockfilters.py test file.
Questions
What happens if the getcfheaders message is malformed and we’re not able
to deserialize it? Try testing this for yourself and looking at the debug
logs.
What changes do we need to make to the ProcessGetCFCheckPt() function in
this PR? Why?
What data is the LookupFilterHashRange() function returning? Why don’t
we just return the filter headers to the peer?
How is the new functionality tested? Should any other tests be added?
<michaelfolkson> I didn't do this. But what was the best approach here to malform it? Randomly tweak it or deliberately construct a message that didn't follow the definition?
<theStack> were there ever any discussions among bitcoin core devs about whether to use exceptions in general? there seem to be very divided views about that in general... same say they are evil because they basically behave like goto/longjmp, and some languages like golang don't even include them (sorry for slight off-topic :))
<theStack> jnewbery: sounds reasonable. in this case it's definitely a good thing that a deserialization return code doesn't need to be checked every time
<theStack> jnewbery: i think this is supposed to be "filter headers" in the second question? (i submitted a PR shortly before the start of the meeting)
<theStack> the idea is that only the one filter header is part of the cfheaders response, the rest are filter _hashes_, and the headers can be computed on the client-side from that information
<fjahr> basically the test does what the client should do as described above, it asks for cfheaders and then validates the hashes it receives by computing that it ends up at the right header
<jkczyz> I came from a world where C++ exception handling was discourage / outright banned. I tend to agree with the reasons behind that. I've come to like variant types where the return value is either the result or an error. Then you never have uninitialized or partially initialized objects around
<theStack> fjahr: yes, you are right. let's say then some languages propose this style pattern in their guidelines _and_ also provide the syntactic sugar for it (to not need an out-parameter, for example), more than others :)
<jnewbery> michaelfolkson: yes, currently only one filter type. BIP 158 initially defined two filter types, but that was reduced to just basic filter types
<michaelfolkson> Re the cfheaders cache size being big enough until 2047. I'm assuming this is a direct trade-off with OOM concerns. Could make it a bit bigger but then you are also raising OOM concerns
<jnewbery> unfortunately discussion about basic and extended filters was before the optech newsletter, so you'll just need to go back to the primary source material!
<michaelfolkson> One final one. I know we are going through these PRs pretty quickly week by week. Do they need longer time to be reviewed? Or are they pretty much ready to all be merged?
<theStack> sipa: yes matching only the public key part of scriptSig data sounds reasonable... the signatures themselves not i think (don't know though how easy it is to distinguish)
<jnewbery> The next PR in the sequence is "Serve cfilters requests", which is pretty much more of the same, so I suggest we don't do a review club on that and reconvene when "Signal NODE_COMPACT_FILTERS support" is open.
<michaelfolkson> Makes sense. Doesn't seem applicable here but generally there is a trade-off of waiting for more reviews versus getting the merging over with.
<fjahr> I also found it a bit unusual that there were merge target date. On the other hand I agree, the code has been out there for a long time, just in different commits
<jnewbery> I put a target date to hold myself accountable to make progress. I'm trying to turn around all review comments on these PRs in less than 24 hours.
<michaelfolkson> I suppose this set of PRs has a unique history of being held up for years due to a couple of Concept NACKs too. If the code has been sitting there for all that time...
<jnewbery> It seems like a monumental waste of effort to have a PR open for 9 months where review happens, there's some delay before the author responds, the reviewer gets bored, the PR needs to be rebased, etc
<theStack> sipa: hmm if you only put sPK data in a block filter you couldn't detect if a block spent an utxo of your interest, or did you mean something different?
<theStack> so far i didn't have the impression that something got merged without a good amount of ACKs -- but i guess it's always a good thing if people stay watchful
<jnewbery> michaelfolkson: I don't think it's true that this was 'held up for years by a couple of concept NACKs'. There were no concept NACKs on the original PR: https://github.com/bitcoin/bitcoin/pull/16442
<michaelfolkson> Yeah interesting. I don't envy the maintainers. We had a Socratic with Jeremy Rubin on CTV and it seems like the design for that proposal has benefitted from the perceived opposition over time
<michaelfolkson> Seems like a much stronger proposal now than if it had been rushed through years ago. But knowing when to delay or oppose versus supporting it being merged seems like a fiendish responsibility