Supercedes #10512 by simply restricting punishments to our outgoing non-feeler connections, and only punishing with a disconnect, not a ban.
This is necessary to avoid banning peers that merely run old formerly-full nodes, after a softfork. We disconnect primary peers because we want compatible full nodes for that role, but allow non-full nodes to remain connected to inbound slots so they can sync the correct chain from us.
This seems like a reasonable approach to me; any idea why stop_node in the zapwallettxes test isn't getting a 0 exit code?
I guess whatever I saw was a transient error then (or I was just confused)...
888 | @@ -865,10 +889,16 @@ void PeerLogicValidation::BlockChecked(const CBlock& block, const CValidationSta 889 | if (state.IsInvalid(nDoS)) { 890 | // Don't send reject message with code 0 or an internal reject code. 891 | if (it != mapBlockSource.end() && State(it->second.first) && state.GetRejectCode() > 0 && state.GetRejectCode() < REJECT_INTERNAL) { 892 | + const NodeId node_id = it->second.first; 893 | + CNodeState * const nodestate = State(node_id); 894 | CBlockReject reject = {(unsigned char)state.GetRejectCode(), state.GetRejectReason().substr(0, MAX_REJECT_MESSAGE_LENGTH), hash}; 895 | State(it->second.first)->rejects.push_back(reject);
nit: Use nodestate instead of State(it->second.first)
Have we considered preferring a relatively short-time-period ban instead of purely disconnecting? If the peer is somehow braindead and tries to reconnect immediately we may prefer to keep them gone for 30 minutes or an hour instead of letting them get into some loop of connect-relay-disconnect.
It was mentioned during the IRC meeting that this already did what I am planning on doing in #11446. However this doesn't seem to cover the case that I am concerned about: receiving a header for a block we have already marked as invalid. In such a case, nDos will be 0 and thus the peer won't be punished for relaying us the duplicate header.
525 | @@ -526,21 +526,6 @@ def run_test(self): 526 | # Remove the first two entries (blocks[1] would connect): 527 | blocks = blocks[2:] 528 | 529 | - # Now try to see how many unconnecting headers we can send
Test passes with the above changes.
101 | + listen_sock = socket.socket() 102 | + listen_sock.bind(('127.0.0.1', 0)) 103 | + listen_sock.listen(1) 104 | + listen_port = listen_sock.getsockname()[1] 105 | + self.rpc.addnode('127.0.0.1:%u' % (listen_port,), 'onetry', False) 106 | + (sock, addr) = listen_sock.accept()
Use (sock, _) = listen_sock.accept() to clarify that the second element in the tuple is discarded intentionally
Re-NACK. While I generally agree with the high-level goals described in the OP, it's not clear what changes here are still necessary to achieve that goal -- several related PRs have been merged in the year it's been since this was originally opened. This PR also seems to break the bugfix made in #8305, without explanation or other handling, which seems unnecessary and unrelated to the overall goal of the PR. And the changes included in this PR seem disjointed (with comments not being updated to reflect new behavior, old nonsensical behavior left in cases that don't make sense, etc) -- presumably because the code changes here have gotten stale? I didn't bother leaving detailed comments in all those cases because after reviewing this, it's not at all clear to me what you're trying to accomplish.
I suggest this PR be closed; if there are any remaining changes that are still relevant and worth proposing, we can evaluate fresh in a new PR. If you think I'm mistaken, please give more detailed documentation and explanation of the reasoning behind these changes (for instance, why does the headers processing logic need to change at all? what cases are you trying to cover, and which cases are still left out?).
Also some of the testing-related changes are separately useful/interesting and warrant their own discussion. In particular I think supporting outbound peers that are not treated as "manual connections" for net processing purposes would be great, but I think those changes should be reviewed on their own -- I'd prefer we split those out into their own PR, rather than have them be an afterthought to support a test in a single PR.
#10593 (comment) ... you never explained how you think it breaks #8305
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--174a7506f384e20aa4161008e828411d-->
Reviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.
feature_block.py fails on all travis jobs that run it
Probably should close this PR; it needs a pretty serious rebase, the RECENT_CONSENSUS_CHANGE stuff from #15141 probably warrants some more thought about the approach which might not happen until we've got a consensus change to go with it, and the current approach has a NACK...
#12674 and the "nodo-to-test connections" support for mininode are probably relevant for #14210 though.
it needs a pretty serious rebase
Rebase is now complete. Push needs to wait for the PR to be reopened, but @sdaftuar is right that it should probably be split up and smaller changes merged first.
the RECENT_CONSENSUS_CHANGE stuff from #15141 probably warrants some more thought about the approach which might not happen until we've got a consensus change to go with it,
Recent consensus changes shouldn't be (at least in this context) treated any differently than other consensus rules. You're looking at it in the limited scenario of a softfork, but it needs to be broader to ensure we overwrite/reorg any attempts to hardfork without consensus too.