Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Relax punishment for peers relaying invalid blocks and headers #10593

Closed
wants to merge 7 commits into from

Conversation

@luke-jr
Copy link
Member

@luke-jr luke-jr commented Jun 14, 2017

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.

@fanquake fanquake added the P2P label Jun 15, 2017
luke-jr added a commit to bitcoinknots/bitcoin that referenced this pull request Jun 15, 2017
luke-jr added a commit to bitcoinknots/bitcoin that referenced this pull request Jun 15, 2017
… we're relying on them as a primary node

Github-Pull: bitcoin#10593
Rebased-From: 35433a2
luke-jr added a commit to luke-jr/bitcoin that referenced this pull request Jun 15, 2017
@ajtowns
Copy link
Contributor

@ajtowns ajtowns commented Jun 20, 2017

This seems like a reasonable approach to me; any idea why stop_node in the zapwallettxes test isn't getting a 0 exit code?

@luke-jr
Copy link
Member Author

@luke-jr luke-jr commented Jul 4, 2017

@ajtowns Not sure what you're asking? The tests are passing...

luke-jr added a commit to luke-jr/bitcoin that referenced this pull request Jul 4, 2017
luke-jr added a commit to luke-jr/bitcoin that referenced this pull request Jul 4, 2017
… we're relying on them as a primary node

Github-Pull: bitcoin#10593
Rebased-From: 35433a2
@ajtowns
Copy link
Contributor

@ajtowns ajtowns commented Jul 5, 2017

I guess whatever I saw was a transient error then (or I was just confused)...

luke-jr added a commit to luke-jr/bitcoin that referenced this pull request Jul 17, 2017
luke-jr added a commit to luke-jr/bitcoin that referenced this pull request Jul 17, 2017
… we're relying on them as a primary node

Github-Pull: bitcoin#10593
Rebased-From: 35433a2

Rebased-From: 73ad56d
luke-jr added a commit to luke-jr/bitcoin that referenced this pull request Jul 17, 2017
@@ -865,10 +889,16 @@ void PeerLogicValidation::BlockChecked(const CBlock& block, const CValidationSta
if (state.IsInvalid(nDoS)) {
// Don't send reject message with code 0 or an internal reject code.
if (it != mapBlockSource.end() && State(it->second.first) && state.GetRejectCode() > 0 && state.GetRejectCode() < REJECT_INTERNAL) {
const NodeId node_id = it->second.first;
CNodeState * const nodestate = State(node_id);
CBlockReject reject = {(unsigned char)state.GetRejectCode(), state.GetRejectReason().substr(0, MAX_REJECT_MESSAGE_LENGTH), hash};
State(it->second.first)->rejects.push_back(reject);

This comment has been minimized.

@jimpo

jimpo Oct 5, 2017
Contributor

nit: Use nodestate instead of State(it->second.first)

@TheBlueMatt
Copy link
Contributor

@TheBlueMatt TheBlueMatt commented Oct 5, 2017

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.

@sdaftuar
Copy link
Member

@sdaftuar sdaftuar commented Oct 6, 2017

It seems like this reverts #8305 with no mechanism to replace that functionality. Rationale? This looks like a regression. NACK.

@achow101
Copy link
Member

@achow101 achow101 commented Oct 7, 2017

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.

@laanwj laanwj modified the milestones: 0.15.1, 0.15.0.2 Oct 12, 2017
@laanwj laanwj added this to Review priority 0.15.0.2 in High-priority for review Oct 12, 2017
@@ -526,21 +526,6 @@ def run_test(self):
# Remove the first two entries (blocks[1] would connect):
blocks = blocks[2:]

# Now try to see how many unconnecting headers we can send

This comment has been minimized.

@promag

promag Oct 16, 2017
Member

Test passes with the above changes.

@laanwj laanwj removed this from the 0.15.0.2 milestone Oct 26, 2017
@laanwj laanwj removed this from Review priority 0.15.0.2 in High-priority for review Oct 26, 2017
@luke-jr luke-jr force-pushed the luke-jr:relax_invblk_punishment branch 2 times, most recently Nov 6, 2017
@luke-jr luke-jr force-pushed the luke-jr:relax_invblk_punishment branch 2 times, most recently Mar 12, 2018
@luke-jr luke-jr force-pushed the luke-jr:relax_invblk_punishment branch to dff2b8d Nov 3, 2018
@DrahtBot DrahtBot removed the Needs rebase label Nov 3, 2018
listen_sock.listen(1)
listen_port = listen_sock.getsockname()[1]
self.rpc.addnode('127.0.0.1:%u' % (listen_port,), 'onetry', False)
(sock, addr) = listen_sock.accept()

This comment has been minimized.

@practicalswift

practicalswift Nov 4, 2018
Member

Use (sock, _) = listen_sock.accept() to clarify that the second element in the tuple is discarded intentionally

Copy link
Member

@sdaftuar sdaftuar left a comment

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.

@luke-jr
Copy link
Member Author

@luke-jr luke-jr commented Nov 6, 2018

#10593 (comment) ... you never explained how you think it breaks #8305

@sdaftuar
Copy link
Member

@sdaftuar sdaftuar commented Nov 6, 2018

@luke-jr The point of #8305 was to avoid giving DoS points to peers for the occasional unconnecting header. This PR would immediately disconnect an outbound peer when it announces a header that doesn't connect.

@DrahtBot
Copy link
Contributor

@DrahtBot DrahtBot commented Nov 9, 2018

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #15141 (Rewrite DoS interface between validation and net_processing by sdaftuar)
  • #14918 (RPCHelpMan: Check default values are given at compile-time by MarcoFalke)

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.

@luke-jr luke-jr force-pushed the luke-jr:relax_invblk_punishment branch from dff2b8d to 117775c Feb 12, 2019
@DrahtBot DrahtBot removed the Needs rebase label Feb 12, 2019
@MarcoFalke
Copy link
Member

@MarcoFalke MarcoFalke commented Feb 12, 2019

feature_block.py fails on all travis jobs that run it

@luke-jr luke-jr force-pushed the luke-jr:relax_invblk_punishment branch from 117775c to 2fff121 Apr 25, 2019
@ajtowns
Copy link
Contributor

@ajtowns ajtowns commented Sep 5, 2019

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.

@luke-jr
Copy link
Member Author

@luke-jr luke-jr commented Oct 16, 2019

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.

@laanwj laanwj removed the Needs rebase label Oct 24, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

You can’t perform that action at this time.