p2p: log mutated blocks when unknown & valid PoW by 0xB10C · Pull Request #29549 · bitcoin/bitcoin

0xB10C · 2024-03-04T12:12:56Z

Follow-up to #29412. Before 29412, we were logging an error when receiving an unknown mutated block with a valid PoW. With 29412, we only log about mutated blocks to the net debug logging category. As the mutated block logging is and has been useful for detecting and debugging problem of some mining pools, it's re-added here. See the conversation starting here: #29412 (comment).

Old blocks can cheaply be mutated and send to us. We don't log when receiving these. However, blocks with a valid PoW (above our DoS threshold) that we didn't know about could indicate a problem, for example, on the miner side. By logging about these mutated blocks, we make it easier to notice and debug these cases.

DrahtBot · 2024-03-04T12:12:59Z

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

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.
A summary of reviews will appear here.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#29538 (refactor: Improve naming of CBlock::GetHash() -> GetHeaderHash() by fjahr)

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.

0xB10C · 2024-03-04T12:12:59Z

I'm interested in conceptual review on:

An attacker could try to fill our disk by sending us the same mutated block that we then log. As they get disconnected after sending a mutated block, the attack frequency is limited. Accepting the header of the mutated block would avoid this, but would partly revert p2p: Don't process mutated blocks #29412, as we would do some processing (accepting the header) on mutated blocks. See also p2p: Don't process mutated blocks #29412 (comment) and following comment.
This adds another log-assertion in the functional tests. We might want to avoid these: p2p: Don't consider blocks mutated if they don't connect to known prev block #29524 (comment)

DrahtBot · 2024-03-04T12:23:21Z

🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the
documentation.

Possibly this is due to a silent merge conflict (the changes in this pull request being
incompatible with the current code in the target branch). If so, make sure to rebase on the latest
commit of the target branch.

Leave a comment here, if you need help tracking down a confusing failure.

_{Debug: https://github.com/bitcoin/bitcoin/runs/22244171302}

fanquake · 2024-03-04T12:35:12Z

net_processing.cpp:4733:53: error: cannot call function 'MaybeCheckNotHeld' while mutex 'cs_main' is held [-Werror,-Wthread-safety-analysis]
                   const CBlockIndex* mutated_block{WITH_LOCK(m_chainman.GetMutex(), return m_chainman.m_blockman.LookupBlockIndex(pblock->GetHash()))};
                                                    ^
./sync.h:301:30: note: expanded from macro 'WITH_LOCK'
#define WITH_LOCK(cs, code) (MaybeCheckNotHeld(cs), [&]() -> decltype(auto) { LOCK(cs); code; }())
                             ^
1 error generated.

Old blocks can cheaply be mutated and send to us. We don't log when receiving these. However, blocks with a valid PoW (above our DoS threshold) that we didn't know about could indicate a problem, for example, on the miner side. By logging about these mutated blocks, we make it easier to notice and debug these cases.

0xB10C · 2024-03-04T13:54:31Z

Dropped the chainman lock. We are already locking cs_main, which should be enough (although I'm not 100% certain I fully understand our locking internals here).

test/functional/p2p_mutated_blocks.py

Sjors · 2024-03-05T10:31:05Z

Somewhat related: #27277

There I'm trying to declutter -debug=validation. That could also be a useful target for the log messages here. It's not on by default - so there's need to worry about disk filling. It's also not a noisy as -debug=net. I marked it as draft though, because over the last few rebases things got a bit confusing.

instagibbs · 2024-03-05T14:04:53Z

src/net_processing.cpp

+            {
+                LOCK(cs_main);
+                RemoveBlockRequest(pblock->GetHash(), peer->m_id);
+                if (prev_block && prev_block->nChainWork + CalculateHeadersWork({pblock->GetBlockHeader()}) >= GetAntiDoSWorkThreshold()) {


Suggested change

if (prev_block && prev_block->nChainWork + CalculateHeadersWork({pblock->GetBlockHeader()}) >= GetAntiDoSWorkThreshold()) {

if (prev_block->nChainWork + CalculateHeadersWork({pblock->GetBlockHeader()}) >= GetAntiDoSWorkThreshold()) {

instagibbs · 2024-03-05T14:20:20Z

test/functional/p2p_mutated_blocks.py

+          expected_msgs=["bad-witness-nonce-size, CheckWitnessMalleation", LOGMESSAGE_REJECTED_MUTATED_BLOCK]):
+            attacker.send_message(msg_block(mutated_block))
+        attacker.wait_for_disconnect(timeout=5)
+


Logic seems wrong, and here's a test which catches it. We'll make an unconditional log for every mutated block, over and over. To mitigate this we would need to accept the header of the mutated block and use that to rate-limit the log.

Suggested change

# But won't log a second time

attacker = self.nodes[0].add_p2p_connection(P2PInterface())

with self.nodes[0].assert_debug_log(

expected_msgs=["Received mutated block from peer"], # have it wait for this to make sure unexpected has time

unexpected_msgs=["bad-witness-nonce-size, CheckWitnessMalleation", LOGMESSAGE_REJECTED_MUTATED_BLOCK]):

attacker.send_message(msg_block(mutated_block))

attacker.wait_for_disconnect(timeout=5)

This was mentioned in #29412 (comment) too. The current implementation in this PR is the same behavior as before #29412. The logging is somewhat rate-limited by needing to open a new connection each time.

I agree that re-introducing this isn't ideal. As this would partially revert #29412 again, I don't think it's worth having this on-by-default logging: #29549 (comment)

dergoegge · 2024-03-04T12:48:04Z

src/net_processing.cpp

This is not an actual proof-of-work check. CalculateHeadersWork only returns the work claimed by the header but doesn't check whether the header actually has valid work. So the log is trivial to trigger by anyone.

that's a very unfortunate name for the function

And it's bitten people before, see e.g. #27278 (comment)

ugh, good catch.

0xB10C · 2024-03-06T08:18:46Z

I came to the conclusion that checking for mutations early (before we even look at the header), isn't trivially compatible with doing anything that's potentially DoSable (e.g. logging), even if we disconnect after each mutated block. As mentioned in #29412 (comment) and #29549 (comment), we would want to check (and accept) the header before logging, which would partially revert #29412. I don't think logging mutated blocks is worth doing this.

Additionally, this is only a problem if the mutated block is submitted over a custom/broken P2P client that doesn't itself check the block. Any miner using submitblock on their node will notice that their block is mutated.

If still wanted, it's probably better to log mutated blocks in debug=validation (with e.g. #27277). I don't plan to work on this at the moment.

eb7cc9f Rename CalculateHeadersWork to CalculateClaimedHeadersWork (Greg Sanders) Pull request description: And clean up some comments. Confusion about what this is doing seems to be a running theme: #29549 (comment) #27278 (comment) ACKs for top commit: achow101: ACK eb7cc9f pablomartin4btc: ACK eb7cc9f 0xB10C: ACK eb7cc9f dergoegge: ACK eb7cc9f BrandonOdiwuor: ACK eb7cc9f Tree-SHA512: 6ccbc5e417155516487bb220753d189b5341dec05366db88a3fa5b1932eace21fbfaf23408c639bb54b36169a8d0a7536a1ee5e63b4ce5a3b70f2ff8407b6e07

DrahtBot added the P2P label Mar 4, 2024

0xB10C force-pushed the 2024-02-log-high-work-mutated-blocks branch from 34e1777 to 5fce7ef Compare March 4, 2024 12:23

DrahtBot added the CI failed label Mar 4, 2024

0xB10C force-pushed the 2024-02-log-high-work-mutated-blocks branch from 5fce7ef to ffd22c8 Compare March 4, 2024 13:41

DrahtBot removed the CI failed label Mar 4, 2024

DrahtBot mentioned this pull request Mar 4, 2024

refactor: Improve naming of CBlock::GetHash() -> GetHeaderHash() #29538

Closed

Sjors reviewed Mar 5, 2024

View reviewed changes

test/functional/p2p_mutated_blocks.py Show resolved Hide resolved

fanquake requested review from dergoegge and instagibbs March 5, 2024 13:40

instagibbs suggested changes Mar 5, 2024

View reviewed changes

dergoegge reviewed Mar 5, 2024

View reviewed changes

instagibbs mentioned this pull request Mar 5, 2024

Rename CalculateHeadersWork to CalculateClaimedHeadersWork #29569

Merged

0xB10C closed this Mar 6, 2024

bitcoin locked and limited conversation to collaborators Mar 6, 2025

	if (prev_block && prev_block->nChainWork + CalculateHeadersWork({pblock->GetBlockHeader()}) >= GetAntiDoSWorkThreshold()) {
	if (prev_block->nChainWork + CalculateHeadersWork({pblock->GetBlockHeader()}) >= GetAntiDoSWorkThreshold()) {

+        # But won't log a second time
+        attacker = self.nodes[0].add_p2p_connection(P2PInterface())
+        with self.nodes[0].assert_debug_log(
+          expected_msgs=["Received mutated block from peer"], # have it wait for this to make sure unexpected has time
+          unexpected_msgs=["bad-witness-nonce-size, CheckWitnessMalleation", LOGMESSAGE_REJECTED_MUTATED_BLOCK]):
+            attacker.send_message(msg_block(mutated_block))
+        attacker.wait_for_disconnect(timeout=5)

Conversation

0xB10C commented Mar 4, 2024

Uh oh!

DrahtBot commented Mar 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage

Reviews

Conflicts

Uh oh!

0xB10C commented Mar 4, 2024

Uh oh!

DrahtBot commented Mar 4, 2024

Uh oh!

fanquake commented Mar 4, 2024

Uh oh!

0xB10C commented Mar 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Sjors commented Mar 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

instagibbs Mar 5, 2024

Choose a reason for hiding this comment

Uh oh!

instagibbs Mar 5, 2024

Choose a reason for hiding this comment

Uh oh!

0xB10C Mar 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dergoegge Mar 4, 2024

Choose a reason for hiding this comment

Uh oh!

instagibbs Mar 5, 2024

Choose a reason for hiding this comment

Uh oh!

Sjors Mar 5, 2024

Choose a reason for hiding this comment

Uh oh!

0xB10C Mar 6, 2024

Choose a reason for hiding this comment

Uh oh!

0xB10C commented Mar 6, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

DrahtBot commented Mar 4, 2024 •

edited

Loading

0xB10C commented Mar 4, 2024 •

edited

Loading

Sjors commented Mar 5, 2024 •

edited

Loading

0xB10C Mar 6, 2024 •

edited

Loading