Page MenuHomePhabricator

Reject blocks that violate the real time target policy
ClosedPublic

Authored by Fabien on Sep 6 2024, 14:00.

Details

Reviewers
PiRK
roqqit
Group Reviewers
Restricted Project
Commits
rABC8356480e591e: Reject blocks that violate the real time target policy
Summary

This diff adds a new policy to reject blocks whose hash is higher than the real time target.
The policy is gated behind a flag and disabled by default, as the node still lacks support for mining compliant blocks.

Depends on D16773.

Test Plan
ninja all check-all

Diff Detail

Repository
rABC Bitcoin ABC
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

Fabien requested review of this revision.Sep 6 2024, 14:00
This revision is now accepted and ready to land.Sep 6 2024, 15:39
roqqit requested changes to this revision.Sep 6 2024, 17:11
roqqit added a subscriber: roqqit.
roqqit added inline comments.
test/functional/abc_p2p_avalanche_policy_rtt.py
114–117 ↗(On Diff #49531)

Reading this without digging into the internals of check_new_block is confusing. The API suggests the new block is rejected but then what immediately follows is a check that it was actually accepted. The easiest fix is the rename check_new_block to something like check_and_accept_new_block with a rename of the expect_accepted parameter to expect_initially_accepted.

121 ↗(On Diff #49531)

This test could be improved to check late blocks as well (now += 1200, 1800, etc). I know this overlaps with the unit tests a bit, but it costs very little to have that extra confidence.

This revision now requires changes to proceed.Sep 6 2024, 17:11

Rebase, feedback and minor adaptation to accomodate the new algo

roqqit requested changes to this revision.Wed, Sep 11, 21:15
roqqit added inline comments.
src/test/rtt_tests.cpp
317–322 ↗(On Diff #49592)

this needs to loop through every block to make this check more comprehensive

This revision now requires changes to proceed.Wed, Sep 11, 21:15
bytesofman added inline comments.
test/functional/abc_p2p_avalanche_policy_rtt.py
117 ↗(On Diff #49592)

so this block is ... not accepted on this line? but accepted on the next line?

not sure what is going on here

test/functional/abc_p2p_avalanche_policy_rtt.py
117 ↗(On Diff #49592)

The block is initially parked by the node for not matching the RTT (the False argument at second position), then accepted by avalanche because the peers vote made the node reconsider its position.

roqqit requested changes to this revision.Thu, Sep 12, 15:41
roqqit added inline comments.
src/test/rtt_tests.cpp
325 ↗(On Diff #49607)

this loop still needs to check block 17

This revision now requires changes to proceed.Thu, Sep 12, 15:41
Fabien requested review of this revision.Fri, Sep 13, 07:12
Fabien added inline comments.
src/test/rtt_tests.cpp
325 ↗(On Diff #49607)

The algo doesn't look for N-17 previous pointer, because it doesn't need it

roqqit added inline comments.
src/policy/block/rtt.cpp
36–37 ↗(On Diff #49607)

This can be moved into the if block below. No need to compute it if it's not used.

src/test/rtt_tests.cpp
325 ↗(On Diff #49607)

right

This revision is now accepted and ready to land.Fri, Sep 13, 19:49

Move difftime where it's used, rebase