Do not send (potentially) invalid headers in response to getheaders #11580

TheBlueMatt · 2017-10-30T14:45:22Z

Nowhere else in the protocol do we send headers which are for
blocks we have not fully validated except in response to getheaders
messages with a null locator. On my public node I have not seen any
such request (whether for an invalid block or not) in at least two
years of debug.log output, indicating that this should have minimal
impact.

promag · 2017-10-30T14:52:41Z

src/net_processing.cpp

@@ -1986,8 +1982,7 @@ bool static ProcessMessage(CNode* pfrom, const std::string& strCommand, CDataStr
                return true;
            pindex = (*mi).second;

-            if (!chainActive.Contains(pindex) &&
-                !StaleBlockRequestAllowed(pindex, chainparams.GetConsensus())) {
+            if (!StaleBlockRequestAllowed(pindex, chainparams.GetConsensus())) {


So it was missing IsValid(BLOCK_VALID_SCRIPTS) here?

"Missing" is a strong word given there isn't exactly a spec for the P2P protocol, but, yes, I'd say it should be there.

Should be tagged backport then?

Meh, dont think it matters too much.

jimpo

Concept ACK

jimpo · 2017-10-30T14:58:14Z

src/net_processing.cpp

@@ -759,7 +759,8 @@ void Misbehaving(NodeId pnode, int howmuch)
 static bool StaleBlockRequestAllowed(const CBlockIndex* pindex, const Consensus::Params& consensusParams)
 {
    AssertLockHeld(cs_main);
-    return (pindexBestHeader != nullptr) &&
+    if (chainActive.Contains(pindex)) return true;


Now that this function is expected to accept main chain blocks, maybe change the function name to BlockRequestAllowed?

jimpo · 2017-10-30T14:58:47Z

src/net_processing.cpp

@@ -759,7 +759,8 @@ void Misbehaving(NodeId pnode, int howmuch)
 static bool StaleBlockRequestAllowed(const CBlockIndex* pindex, const Consensus::Params& consensusParams)
 {
    AssertLockHeld(cs_main);
-    return (pindexBestHeader != nullptr) &&
+    if (chainActive.Contains(pindex)) return true;
+    return pindex->IsValid(BLOCK_VALID_SCRIPTS) && (pindexBestHeader != nullptr) &&


Update the function comment to describe this condition.

Nowhere else in the protocol do we send headers which are for blocks we have not fully validated except in response to getheaders messages with a null locator. On my public node I have not seen any such request (whether for an invalid block or not) in at least two years of debug.log output, indicating that this should have minimal impact.

TheBlueMatt · 2017-10-30T23:00:29Z

Addressed @jimpo's comments.

gmaxwell · 2017-11-01T00:56:31Z

utACK

ryanofsky

I wrote a test for this change which you could include here: e79a9c9.

ACK 3788a84

…ated

TheBlueMatt · 2017-11-02T17:49:23Z

Thanks @ryanofsky!

laanwj · 2017-11-09T18:58:38Z

utACK 725b79a, thanks for adding the test @ryanofsky

…o getheaders 725b79a [test] Verify node doesn't send headers that haven't been fully validated (Russell Yanofsky) 3788a84 Do not send (potentially) invalid headers in response to getheaders (Matt Corallo) Pull request description: Nowhere else in the protocol do we send headers which are for blocks we have not fully validated except in response to getheaders messages with a null locator. On my public node I have not seen any such request (whether for an invalid block or not) in at least two years of debug.log output, indicating that this should have minimal impact. Tree-SHA512: c1f6e0cdcdfb78ea577d555f9b3ceb1b4b60eff4f6cf313bfd8b576c9562d797bea73abc23f7011f249ae36dd539c715f3d20487ac03ace60e84e1b77c0c1e1a

…g entries be9f38c Do not make it trivial for inbound peers to generate log entries (Matt Corallo) Pull request description: Based on #11580 because I'm lazy. We should generally avoid writing to debug.log unconditionally for inbound peers which misbehave (the peer being about to be banned being an exception, since they cannot do this twice). Tree-SHA512: 8e59c8d08d00b1527951b30f4842d010a4c2fc440503ade112baa2c1b9afd0e0d1c5c2df83dde25183a242af45089cf9b9f873b71796771232ffb6c5fc6cc0cc

…ponse to getheaders 725b79a [test] Verify node doesn't send headers that haven't been fully validated (Russell Yanofsky) 3788a84 Do not send (potentially) invalid headers in response to getheaders (Matt Corallo) Pull request description: Nowhere else in the protocol do we send headers which are for blocks we have not fully validated except in response to getheaders messages with a null locator. On my public node I have not seen any such request (whether for an invalid block or not) in at least two years of debug.log output, indicating that this should have minimal impact. Tree-SHA512: c1f6e0cdcdfb78ea577d555f9b3ceb1b4b60eff4f6cf313bfd8b576c9562d797bea73abc23f7011f249ae36dd539c715f3d20487ac03ace60e84e1b77c0c1e1a

…rate log entries be9f38c Do not make it trivial for inbound peers to generate log entries (Matt Corallo) Pull request description: Based on bitcoin#11580 because I'm lazy. We should generally avoid writing to debug.log unconditionally for inbound peers which misbehave (the peer being about to be banned being an exception, since they cannot do this twice). Tree-SHA512: 8e59c8d08d00b1527951b30f4842d010a4c2fc440503ade112baa2c1b9afd0e0d1c5c2df83dde25183a242af45089cf9b9f873b71796771232ffb6c5fc6cc0cc

fanquake added the P2P label Oct 30, 2017

promag reviewed Oct 30, 2017

View reviewed changes

jimpo reviewed Oct 30, 2017

View reviewed changes

TheBlueMatt force-pushed the 2017-10-getheaders-valid-only branch 2 times, most recently from 7344795 to 3788a84 Compare October 30, 2017 22:59

TheBlueMatt mentioned this pull request Oct 30, 2017

Do not make it trivial for inbound peers to generate log entries #11583

Merged

ryanofsky reviewed Nov 1, 2017

View reviewed changes

[test] Verify node doesn't send headers that haven't been fully valid…

725b79a

…ated

laanwj merged commit 725b79a into bitcoin:master Nov 9, 2017

codablock mentioned this pull request Mar 12, 2019

Backport PRs from Bitcoin required to make ActivateBestChain cs_main free on entry dashpay/dash#2764

Merged

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Do not send (potentially) invalid headers in response to getheaders #11580

Do not send (potentially) invalid headers in response to getheaders #11580

TheBlueMatt commented Oct 30, 2017

promag Oct 30, 2017

TheBlueMatt Oct 30, 2017

promag Oct 30, 2017

TheBlueMatt Oct 30, 2017

jimpo left a comment

jimpo Oct 30, 2017

jimpo Oct 30, 2017

TheBlueMatt commented Oct 30, 2017

gmaxwell commented Nov 1, 2017

ryanofsky left a comment

TheBlueMatt commented Nov 2, 2017 •

edited

laanwj commented Nov 9, 2017

Do not send (potentially) invalid headers in response to getheaders #11580

Do not send (potentially) invalid headers in response to getheaders #11580

Conversation

TheBlueMatt commented Oct 30, 2017

promag Oct 30, 2017

Choose a reason for hiding this comment

TheBlueMatt Oct 30, 2017

Choose a reason for hiding this comment

promag Oct 30, 2017

Choose a reason for hiding this comment

TheBlueMatt Oct 30, 2017

Choose a reason for hiding this comment

jimpo left a comment

Choose a reason for hiding this comment

jimpo Oct 30, 2017

Choose a reason for hiding this comment

jimpo Oct 30, 2017

Choose a reason for hiding this comment

TheBlueMatt commented Oct 30, 2017

gmaxwell commented Nov 1, 2017

ryanofsky left a comment

Choose a reason for hiding this comment

TheBlueMatt commented Nov 2, 2017 • edited

laanwj commented Nov 9, 2017

TheBlueMatt commented Nov 2, 2017 •

edited