Send transactions after a CMerkleBlock when asked for it in an inv.

TheBlueMatt commented at 6:56 pm on January 18, 2013: member

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.

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.

28b80e6065

Replace 520 constant with MAX_SCRIPT_ELEMENT_SIZE 192cc910ec

BitcoinPullTester commented at 7:11 pm on January 18, 2013: none

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/192cc910ec7cade1d0dce7f3b111e7fc7720e607 for binaries and test log.

in src/script.h: in 192cc910ec

16@@ -17,6 +17,8 @@
17 class CCoins;
18 class CTransaction;
19 
20+static const unsigned int MAX_SCRIPT_ELEMENT_SIZE = 520; // bytes

Diapolo commented at 11:17 pm on January 18, 2013:

I like such constants and think we have too many hard-coded numbers in the code.

TheBlueMatt commented at 11:23 pm on January 18, 2013:

Yea, sorry I forgot to do this before it got merged (when you asked for it).

gavinandresen commented at 6:08 pm on January 21, 2013: contributor

ACK

sipa commented at 11:08 am on January 22, 2013: member

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.

mikehearn commented at 9:42 pm on January 22, 2013: contributor

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.

gmaxwell commented at 9:55 pm on January 22, 2013: contributor

@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?

mikehearn commented at 10:40 pm on January 22, 2013: contributor

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.

TheBlueMatt commented at 10:49 pm on January 22, 2013: member

@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.

gmaxwell commented at 10:50 pm on January 22, 2013: contributor

If you’re bulk pulling the chain why do you need to serialize getting the transactions with getting the next block?

mikehearn commented at 11:01 pm on January 22, 2013: contributor

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.

sipa commented at 11:40 pm on January 22, 2013: member

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…

gavinandresen referenced this in commit 1a2e45d8d5 on Jan 23, 2013

gavinandresen merged this on Jan 23, 2013

gavinandresen closed this on Jan 23, 2013

laudney referenced this in commit 1fb96ba1bd on Mar 19, 2014

DrahtBot locked this on Sep 8, 2021

Send transactions after a CMerkleBlock when asked for it in an inv. #2188