This relaxes the DoS rules (so cannot cause a network split), so nodes that relay blocks with valid proof-of-work but contain an invalid transaction are not banned.
See issue #3195 and the comment for the rationale.
Also: my OCD kicked in and I couldn't resist fixing a double-negative #error
This relaxes the DoS rules (so cannot cause a network split), so nodes that
relay blocks with valid proof-of-work but contain an invalid transaction
are not banned.
See issue #3195 for the rationale.
2312 | + // valid proof-of-work does not result in the relaying node getting banned. 2313 | + // We do this so that nodes can, in the future, relay blocks as soon 2314 | + // as the basic, fast checks in CheckBlock are done and not risk being 2315 | + // DoS-banned if the more expensive AcceptBlock checks find an invalid 2316 | + // transaction. 2317 | + // There is safe because it would be insanely expensive for an attacker
Typo?
27 | @@ -28,7 +28,7 @@ 28 | using namespace boost; 29 | 30 | #if defined(NDEBUG) 31 | -# error "Bitcoin cannot be compiled without assertions." 32 | +# error "Bitcoin must be compiled with assertions enabled."
The previous sentence used up less storage space...
Github would also use less storage space without your pointless comment :)
LOL
This looks good to me. What's needed to merge it? More reviews?
Yes, a few more reviews and ACKs would be nice. It is a small change, but it does affect main.cpp.
So the only concern here is that it makes DoS protection solely rely on hardness of PoW. That is probably safe, but it breaks the de facto rule on the network of not ever relaying data that is not known to be correct.
An alternative (first suggested by @maaku, I think) is just fast-relaying block headers as soon as they are validated, and using header-first-seen as tie breaker for choosing active chains in case of a fork. This does not mean deviating from that de facto rule, can be extremely fast (it doesn't even need the inv/getdata roundtrip, and doesn't need the block be available to be queried). I wonder what the consequences could be of someone broadcasting their header, and waiting a bit before broadcasting the block, though.
Yes, as soon as headers-first is ready (I'm willing to help on that), the better solution is to fast-relay headers and still DoS-ban on a bad block once it is finally fetched (DoS the peer you got the block from, not the block header).
I don't think we should be serving blocks where we are unsure of the validity identically to blocks that we've validated. Handing bad blocks to SPV nodes, for example, risks creating a network partition between full nodes and SPV nodes even where the SPV nodes are connected only to honest good nodes.
Failing to hang up on nodes giving us bad blocks (which they claim are good and verified) also means that in the case where there is a rule inconsistency the network may end up partitioned simply because nodes do not disconnect peers with the wrong rules and go find ones that agree with them. These are all concerns which should be increasing because mining on alternative implementations is now being promoted (https://twitter.com/dajohi/status/455765633032916992).
Luke had a pull a while back that IIRC added a new unchecked getblock. I think thats a better path: Don't hang up on an invalid block when we asked the peer for an untested block, do hang up when we asked for a tested one. SPV clients that count on full nodes verifying for them would just never request untested blocks.
How about adding a message that means 'I am fine with being told about blocks that are not necessarily valid', which is sent by new nodes at startup to new protocol version peers. SPV nodes or block analysis/wallet watching implementation without validation support would not set it?
Could that also have a "I have now successfully validated block XYZ" message that gets sent when validation is complete to peers who've we've send unvalidated blocks to? If so— we'd use that to find different peers if we have a peer that is obviously imposing different validation rules than us.
In which case, I think thats basically isomorphic to the alternative getblock and seems reasonable to me.
A future change to relay invalid blocks with valid PoW may be controversial, but I see no problem with this PR. If a node sends us invalid blocks with valid PoW, it's unlikely that that node is performing a DoS attack. Banning the node for DoS seems wrong for this reason.
Besides enabling future changes, this change might also help transaction propagation in a hardfork by preventing a network split.
@leofidus No, it can make a spit significantly worse, because a peer running the majority code may be partitioned by minority nodes which do not relay blocks it will accept. That is hardly does-no-harm. It could also enable theft from SPV clients who accept transactions from blocks believing them to be valid and that their full node peers are protecting them by validing blocks. Again, that is hardly do-no-harm.
Note how this can be used to help force a blocksize increase through - the nodes that did not increase the blocksize will still relay invalid and oversized blocks for the ones that did. (up to 20MiB, the max serialization limit)
@petertodd: If I understand the idea correctly (@gavinandresen correct me if I'm wrong), it would mean relaying blocks as soon as the header plus transactions have been received (and the basic checks on those have been performed, but before doing the full (utxo looking, signature check) validity. As you obviously know the size of what you're going to relay before relaying it, it should be easy enough (and advisable, imho) to check that before relaying.
Yup, I believe an easy fix would be to reduce the MAX_SIZE constant from 20MiB to 1MB. (+ some headroom?) That would have also make the "padded tx" attack last year much less severe.
@petertodd That too, but it's not even necessary. Just only relay after doing the header + transactions checks (which includes the 1MB limitation), but before the signature/utxo validation. Of course, that means no DoS banning senders of overly large blocks, but I don't really care about that nearly as much as an evil miner being able to make the entire network relay an obviously-violating-consensus rules 20 MB block.
@sipa Actually, looking at this again I think I misunderstood the logic in the patch, and it looks like CheckBlock() failing will still cause a DoS ban, so at least what this particular patch implements is ok re: blocksize. (in the sense that it can only be safely followed up by a patch that does CheckBlock() and PoW checks prior to relaying) gmaxwells arguments against this change still apply though.
Automatic sanity-testing: FAILED MERGE, see http://jenkins.bluematt.me/pull-tester/p3580_b9c3ede02922ab4843c356c0f3df2b1618c73ae5/ for test log.
This pull does not merge cleanly onto current master This test script verifies pulls every time they are updated. It, however, dies sometimes and fails to test properly. If you are waiting on a test, please check timestamps to verify that the test.log is moving at http://jenkins.bluematt.me/pull-tester/current/ Contact BlueMatt on freenode if something looks broken.
No consensus, and I don't care enough to work more on this.