Send transactions after a CMerkleBlock when asked for it in an inv. #2188
pull TheBlueMatt wants to merge 2 commits into bitcoin:master from TheBlueMatt:bloom changing 3 files +6 −4-
TheBlueMatt commented at 6:56 pm on January 18, 2013: memberThis 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.
-
Send transactions after a CMerkleBlock when asked for it in an inv.
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.
-
Replace 520 constant with MAX_SCRIPT_ELEMENT_SIZE 192cc910ec
-
BitcoinPullTester commented at 7:11 pm on January 18, 2013: noneAutomatic 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: contributorACKsipa commented at 11:08 am on January 22, 2013: memberI 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: contributorGiven 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: contributorThe 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: contributorIf 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: contributorIt 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: memberAs 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, 2013gavinandresen merged this on Jan 23, 2013gavinandresen closed this on Jan 23, 2013
laudney referenced this in commit 1fb96ba1bd on Mar 19, 2014DrahtBot locked this on Sep 8, 2021
This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-11-24 21:12 UTC
More mirrored repositories can be found on mirror.b10c.me