Send transactions after a CMerkleBlock when asked for it in an inv.#2188
Send transactions after a CMerkleBlock when asked for it in an inv.#2188gavinandresen merged 2 commits intobitcoin:masterfrom
Conversation
This actually simplifies some SPV code, as they can keep track of a filtered block and its txn before accepting both in one step. The previous argument was that SPV nodes should handle the txn the same as any other free txn and then mark them as connected to a block when they get the filtered block itself. However, it now appears that SPV nodes will need to put in more effort to verify loose txn than they would to verify txn in blocks, thus making it more approriate to send the txn after the filtered block.
|
Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/192cc910ec7cade1d0dce7f3b111e7fc7720e607 for binaries and test log. |
There was a problem hiding this comment.
I like such constants and think we have too many hard-coded numbers in the code.
There was a problem hiding this comment.
Yea, sorry I forgot to do this before it got merged (when you asked for it).
|
ACK |
|
I have no problems with this change or its implementation specifically, but I think it shows how much the protocol locks us into a specific implementation. I think it would be cleaner if there was no special casing to work around the latency problem, and txids sent as filtered blocks were treated as normal invs, with their data being stored in the relay cache (ready to be getdata'd the normal way). That doesn't have the latency advantages, though. |
|
Given that performance is the primary reason for this change, I'm not very keen on adding latency for cleanness reasons. Anyway, this work is done for now. Better protocols can come in future. Let's get it merged and launched so end users can benefit. |
|
@mikehern this response make it feel to me that you're now railroading the change. Personally, this is triggering a reflexive negative reaction. I think the fact that you feel the need to defend it this way is a sign that it may be too immature to include. Better protocols can come in the future, but old ones need to be supported a long time. WRT latency, you're talking about 1x RTT for an event that happens once per ten minutes in the steady state. Or an overhead or a 0.01% performance difference for 100ms RTT. Am I missing something there? |
|
The standard way to use SPV clients, at least for now, is to start them up when you need them and shut them down when done. Keeping it running 24/7 doesn't make a whole lot of sense especially on a phone. So the steady state is synchronizing the chain as fast as possible. That's why latency matters. |
|
@gmaxwell Im not so sure that the whole bloom filter stuff is/was railroaded...actually it was pretty well thought/implemented through. This pull itself one could argue is being railroaded, but only because it doesnt make sense for the bloom code as-is to be included in any releases (in part due to changes to bitcoinj that are also only just now happening). Also, the minute difference that this pull represents, IMHO, is really not something that merits a large discussion. |
|
If you're bulk pulling the chain why do you need to serialize getting the transactions with getting the next block? |
|
It reduces memory requirements during chain sync. Anyway, I disagree that requiring a getdata is better. The node knows exactly what comes next after sending the filtered blocks. A getdata is superfluous. WRT "railroading". The code we have works fine. This tweak is not essential but simplifies the code on the client side when various changes that I just posted about to bitcoin-security are implemented. There's no reason not to do it. More generally - perfect is the enemy of the good. Bitcoin does not have infinite time. It isn't something to polish in an ivory tower, after all, Bitcoin already came to the world in a far from perfect state. If SPV clients have acceptable performance 5 years from now it will be irrelevant, people will have either given up on Bitcoin or all be using ad-hoc protocols that talk to different kinds of servers, Electrum style. Forward progress is essential and these changes have taken too long already. |
|
As I didn't want to restart the discussion about this, I probably shouldn't have brought it up again. I believe the Bloom filtering was well thought-out, and the reason for sending transactions immediately was also clear. Reducing latency for mobile clients was a design goal, and that outweighs neatness. I'm fine with this change (to be honest, I assumed this was how it was implemented in the first place). I just wanted to mention that the fact that we need to change something like this because of an implementation issue, makes me feel uneasy. But perhaps there are others who feel the same way... |
Send transactions after a CMerkleBlock when asked for it in an inv.
Send transactions after a CMerkleBlock when asked for it in an inv.
7 participants
This actually simplifies some SPV code, as they can keep track of
a filtered block and its txn before accepting both in one step.
The previous argument was that SPV nodes should handle the txn the
same as any other free txn and then mark them as connected to a
block when they get the filtered block itself. However, it now
appears that SPV nodes will need to put in more effort to verify
loose txn than they would to verify txn in blocks, thus making it
more approriate to send the txn after the filtered block.