fix: miner delay attack #5582
Merged
fix: miner delay attack #5582
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
sdbondi
reviewed
Jul 10, 2023
ghpbot-tari-project
added
P-acks_required
|
Is it possible to label this PR to reflect its status as an audit fix? It should be easy for interested researchers and developers to quickly find audit-related fixes. |
There was a problem hiding this comment.
I think this is not a bad start.
- I like doing the reconcile before the semaphore, but I think you should remove the semaphore entirely since it no longer serves a purpose.
- This solution introduces another problem, as @sdbondi noted, we will be requesting many blocks. I suggest only doing one reconcile per block hash. This could be done using a hashmap/set in this file, or potentially even in
base_node_service.rs::spawn_handle_incoming_blockand only spawning a thread if there is no thread currently handling this blockhash
|
I disagree about removing the semaphore, after this point, we are making changes to the tip, and rely on the tip to stay constant during the process. |
SWvheerden
force-pushed
the
sw_fix_miner_bug
branch
from
37c01a2 to
3f570ed
Compare
base_layer/core/src/base_node/comms_interface/inbound_handlers.rs
Outdated
Show resolved
Hide resolved
sdbondi
reviewed
Jul 24, 2023
There was a problem hiding this comment.
An attacker may still delay the reconciliation if it is the first message, in the worst case making the base node switch to BlockSync mode. This may be fine as an attacker would need to be the first message in, and we assume they have no control over that.
The hash removal also needs to be done in the error case.
base_layer/core/src/base_node/comms_interface/inbound_handlers.rs
Outdated
Show resolved
Hide resolved
base_layer/core/src/base_node/comms_interface/inbound_handlers.rs
Outdated
Show resolved
Hide resolved
base_layer/core/src/base_node/comms_interface/inbound_handlers.rs
Outdated
Show resolved
Hide resolved
base_layer/core/src/base_node/comms_interface/inbound_handlers.rs
Outdated
Show resolved
Hide resolved
base_layer/core/src/base_node/comms_interface/inbound_handlers.rs
Outdated
Show resolved
Hide resolved
sdbondi
reviewed
Jul 24, 2023
base_layer/core/src/base_node/comms_interface/inbound_handlers.rs
Outdated
Show resolved
Hide resolved
SWvheerden
force-pushed
the
sw_fix_miner_bug
branch
2 times, most recently
from
2f010e8 to
1d0a4a6
Compare
sdbondi
reviewed
Jul 25, 2023
base_layer/core/src/base_node/comms_interface/inbound_handlers.rs
Outdated
Show resolved
Hide resolved
sdbondi
reviewed
Aug 1, 2023
SWvheerden
force-pushed
the
sw_fix_miner_bug
branch
2 times, most recently
from
6a7db77 to
ac95868
Compare
sdbondi
reviewed
Aug 2, 2023
base_layer/core/src/base_node/comms_interface/inbound_handlers.rs
Outdated
Show resolved
Hide resolved
SWvheerden
force-pushed
the
sw_fix_miner_bug
branch
from
99edeff to
0b57384
Compare
sdbondi
approved these changes
Aug 8, 2023
ghpbot-tari-project
removed
the
P-reviews_required
SWvheerden
force-pushed
the
sw_fix_miner_bug
branch
from
0b57384 to
0b721c5
Compare
Co-authored-by: Stan Bondi <[email protected]>
SWvheerden
force-pushed
the
sw_fix_miner_bug
branch
from
0b721c5 to
5b6e6e7
Compare
sdbondi
added a commit
to sdbondi/tari
that referenced
this pull request
Aug 10, 2023
…-addresses * development: chore: fix windows install (tari-project#5616) feat: ban peer unexpected response (tari-project#5608) fix!: add validator mr to mining hash (tari-project#5615) fix: check bytes remaining on monero blocks (tari-project#5610) fix: duplicate tari header in monero coinbase (tari-project#5604) fix: monero fork attack (tari-project#5603) feat: add mempool min fee (tari-project#5606) chore(tests): large block unit tests (tari-project#5599) fix: miner delay attack (tari-project#5582) fix: peer connection to stale nodes (tari-project#5579) ci(fix): artifact cleanup for diag-utils (tari-project#5613) ci(fix): update Windows installer for Minotari (tari-project#5614) chore: fixes monero build (tari-project#5612)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Stops the node blocking until it has a full block
Audit Finding Number
TARI-0001, TARI-0002
Motivation and Context
Its possible for a malicious node to block access to the full block by using first the mempool to say it does not have the missing transactions, then when asked for the full block to only then provide the full block. But these requests can be made slow to delay the node constructing the new block by almost 2 mins. This is the block time.
By only accepting 1 block request at a time, a malicious node can lock down the local node for that entire 2 mins knowing they wont accept any other blocks. This changes it so that nodes can still process new requests. The first complete block will be served first.
Also changes the display of block information to ensure block sorted is correctly used.