Page MenuHomePhabricator

Add a block policy to reject turbo blocks
AbandonedPublicDraft

Authored by Fabien on Jun 17 2024, 14:50.

Details

Reviewers
None
Group Reviewers
Restricted Project
Summary

This is a draft of the heartbeat feature. When -enablertt=1, the node will reject blocks that are received too soon after the previous one.
Based on the research from https://ledger.pitt.edu/ojs/ledger/article/download/195/187/1008.

This needs to be split into pieces.

Test Plan
ninja all check check-functional

Diff Detail

Event Timeline

Tail of the build log:

[585/638] Building CXX object src/bench/CMakeFiles/bitcoin-bench.dir/chacha_poly_aead.cpp.o
[586/638] Building CXX object src/bench/CMakeFiles/bitcoin-bench.dir/chacha20.cpp.o
[587/638] Building CXX object src/qt/CMakeFiles/bitcoin-qt-base.dir/bitcoin-qt-base_autogen/mocs_compilation.cpp.o
[588/638] Building CXX object src/qt/CMakeFiles/bitcoin-qt-base.dir/intro.cpp.o
[589/638] Building CXX object src/bench/CMakeFiles/bitcoin-bench.dir/checkqueue.cpp.o
[590/638] Building CXX object src/qt/CMakeFiles/bitcoin-qt-base.dir/optionsdialog.cpp.o
[591/638] Building CXX object src/qt/CMakeFiles/bitcoin-qt-base.dir/bitcoingui.cpp.o
[592/638] Building CXX object src/bench/CMakeFiles/bitcoin-bench.dir/crypto_hash.cpp.o
[593/638] Building CXX object src/bench/CMakeFiles/bitcoin-bench.dir/data.cpp.o
[594/638] Building CXX object src/bench/CMakeFiles/bitcoin-bench.dir/examples.cpp.o
[595/638] Building CXX object src/CMakeFiles/bitcoinkernel.dir/validation.cpp.o
[596/638] Building CXX object src/bench/CMakeFiles/bitcoin-bench.dir/gcs_filter.cpp.o
[597/638] Building CXX object src/qt/CMakeFiles/bitcoin-qt-base.dir/optionsmodel.cpp.o
[598/638] Building CXX object src/bench/CMakeFiles/bitcoin-bench.dir/hashpadding.cpp.o
[599/638] Building CXX object src/bench/CMakeFiles/bitcoin-bench.dir/lockedpool.cpp.o
[600/638] Linking CXX static library src/libbitcoinkernel.a
[601/638] Building CXX object src/qt/CMakeFiles/bitcoin-qt-base.dir/guiutil.cpp.o
[602/638] Building CXX object src/qt/CMakeFiles/bitcoin-qt-base.dir/bitcoin.cpp.o
[603/638] Building CXX object src/bench/CMakeFiles/bitcoin-bench.dir/merkle_root.cpp.o
[604/638] Building CXX object src/bench/CMakeFiles/bitcoin-bench.dir/nanobench.cpp.o
[605/638] Building CXX object src/bench/CMakeFiles/bitcoin-bench.dir/pool.cpp.o
[606/638] Linking CXX executable src/bitcoin-chainstate
FAILED: src/bitcoin-chainstate 
: && /usr/bin/c++ -Werror -g -O2 -fuse-ld=gold -Wl,-z,noexecstack -Wl,-z,relro -Wl,-z,now -lstdc++fs -fPIE -pie src/CMakeFiles/bitcoin-chainstate.dir/bitcoin-chainstate.cpp.o -o src/bitcoin-chainstate  /usr/lib/x86_64-linux-gnu/libjemalloc_pic.a  -lstdc++fs  src/libbitcoinkernel.a  src/crypto/libcrypto.a  src/crypto/libcrypto_sse4.1.a  src/crypto/libcrypto_avx2.a  src/crypto/libcrypto_shani.a  src/univalue/libunivalue.a  src/secp256k1/libsecp256k1.a  src/leveldb/libleveldb.a  src/leveldb/libleveldb-sse4.2.a  src/leveldb/libmemenv.a  /usr/lib/x86_64-linux-gnu/libjemalloc_pic.a  -lm  -pthread  -ldl  -lstdc++fs && :
../../src/./policy/block/rtt.h:30: error: undefined reference to 'vtable for RTTPolicy'
/usr/bin/ld.gold: the vtable symbol may be undefined because the class is missing its key function
collect2: error: ld returned 1 exit status
[607/638] Building CXX object src/bench/CMakeFiles/bitcoin-bench.dir/poly1305.cpp.o
[608/638] Building CXX object src/bench/CMakeFiles/bitcoin-bench.dir/prevector.cpp.o
[609/638] Building CXX object src/bench/CMakeFiles/bitcoin-bench.dir/rollingbloom.cpp.o
[610/638] Building CXX object src/qt/CMakeFiles/bitcoin-qt-base.dir/peertablemodel.cpp.o
[611/638] Building CXX object src/bench/CMakeFiles/bitcoin-bench.dir/block_assemble.cpp.o
[612/638] Building CXX object src/bench/CMakeFiles/bitcoin-bench.dir/streams_findbyte.cpp.o
[613/638] Building CXX object src/bench/CMakeFiles/bitcoin-bench.dir/util_time.cpp.o
[614/638] Building CXX object src/bench/CMakeFiles/bitcoin-bench.dir/verify_script.cpp.o
[615/638] Building CXX object src/bench/CMakeFiles/bitcoin-bench.dir/crypto_aes.cpp.o
[616/638] Building CXX object src/qt/CMakeFiles/bitcoin-qt-base.dir/utilitydialog.cpp.o
[617/638] Building CXX object src/bench/CMakeFiles/bitcoin-bench.dir/checkblock.cpp.o
[618/638] Building CXX object src/qt/CMakeFiles/bitcoin-qt-base.dir/qrc_bitcoin_locale.cpp.o
[619/638] Building CXX object src/qt/CMakeFiles/bitcoin-qt-base.dir/qrc_bitcoin.cpp.o
[620/638] Building CXX object src/qt/CMakeFiles/bitcoin-qt-base.dir/splashscreen.cpp.o
[621/638] Building CXX object src/bench/CMakeFiles/bitcoin-bench.dir/load_external.cpp.o
[622/638] Building CXX object src/bench/CMakeFiles/bitcoin-bench.dir/duplicate_inputs.cpp.o
[623/638] Building CXX object src/bench/CMakeFiles/bitcoin-bench.dir/chained_tx.cpp.o
[624/638] Building CXX object src/bench/CMakeFiles/bitcoin-bench.dir/mempool_eviction.cpp.o
[625/638] Building CXX object src/qt/CMakeFiles/bitcoin-qt-base.dir/rpcconsole.cpp.o
[626/638] Building CXX object src/bench/CMakeFiles/bitcoin-bench.dir/peer_eviction.cpp.o
[627/638] Building CXX object src/bench/CMakeFiles/bitcoin-bench.dir/bench.cpp.o
[628/638] Building CXX object src/bench/CMakeFiles/bitcoin-bench.dir/mempool_stress.cpp.o
[629/638] Building CXX object src/bench/CMakeFiles/bitcoin-bench.dir/rpc_mempool.cpp.o
[630/638] Building CXX object src/bench/CMakeFiles/bitcoin-bench.dir/rpc_blockchain.cpp.o
[631/638] Building CXX object src/qt/CMakeFiles/bitcoin-qt-base.dir/trafficgraphwidget.cpp.o
[632/638] Linking CXX static library src/qt/libbitcoin-qt-base.a
[633/638] Automatic MOC for target bitcoin-qt
[634/638] Building CXX object src/qt/CMakeFiles/bitcoin-qt.dir/bitcoin-qt_autogen/mocs_compilation.cpp.o
[635/638] Linking CXX executable src/bench/bitcoin-bench
[636/638] Building CXX object src/qt/CMakeFiles/bitcoin-qt.dir/main.cpp.o
[637/638] Linking CXX executable src/qt/bitcoin-qt
ninja: build stopped: cannot make progress due to previous errors.
Build build-without-wallet failed with exit code 1

Tail of the build log:

chronik_script_confirmed_txs.py            | ○ Skipped | 0 s
chronik_script_history.py                  | ○ Skipped | 0 s
chronik_script_unconfirmed_txs.py          | ○ Skipped | 0 s
chronik_script_utxos.py                    | ○ Skipped | 0 s
chronik_serve.py                           | ○ Skipped | 0 s
chronik_spent_by.py                        | ○ Skipped | 0 s
chronik_token_alp.py                       | ○ Skipped | 0 s
chronik_token_broadcast_txs.py             | ○ Skipped | 0 s
chronik_token_burn.py                      | ○ Skipped | 0 s
chronik_token_id_group.py                  | ○ Skipped | 0 s
chronik_token_parse_failure.py             | ○ Skipped | 0 s
chronik_token_script_group.py              | ○ Skipped | 0 s
chronik_token_slp_fungible.py              | ○ Skipped | 0 s
chronik_token_slp_mint_vault.py            | ○ Skipped | 0 s
chronik_token_slp_nft1.py                  | ○ Skipped | 0 s
chronik_tx.py                              | ○ Skipped | 0 s
chronik_tx_removal_order.py                | ○ Skipped | 0 s
chronik_ws.py                              | ○ Skipped | 0 s
chronik_ws_ordering.py                     | ○ Skipped | 0 s
chronik_ws_ping.py                         | ○ Skipped | 0 s
chronik_ws_script.py                       | ○ Skipped | 0 s
feature_bind_port_discover.py              | ○ Skipped | 0 s
feature_bind_port_externalip.py            | ○ Skipped | 0 s
interface_usdt_net.py                      | ○ Skipped | 0 s
interface_usdt_utxocache.py                | ○ Skipped | 0 s
interface_usdt_validation.py               | ○ Skipped | 0 s

ALL                                        | ✓ Passed  | 3104 s (accumulated) 
Runtime: 623 s

[168/509] Test Bitcoin RPC authentication...
...
----------------------------------------------------------------------
Ran 3 tests in 0.005s

OK
[169/509] cd /work/contrib/devtools/chainparams && /usr/bin/python3.9 ./test_make_chainparams.py
.....
----------------------------------------------------------------------
Ran 5 tests in 0.001s

OK
[237/509] bitcoin: testing rtt_tests
FAILED: src/test/CMakeFiles/check-bitcoin-rtt_tests 
cd /work/abc-ci-builds/build-debug/src/test && /usr/bin/cmake -E make_directory /work/abc-ci-builds/build-debug/test/junit && /usr/bin/cmake -E make_directory /work/abc-ci-builds/build-debug/test/log && /usr/bin/cmake -E env /work/cmake/utils/log-and-print-on-failure.sh /work/abc-ci-builds/build-debug/test/log/bitcoin-rtt_tests.log /work/abc-ci-builds/build-debug/src/test/test_bitcoin --run_test=rtt_tests --logger=HRF,message:JUNIT,message,bitcoin-rtt_tests.xml --catch_system_errors=no
../../src/policy/block/rtt.cpp:83 GetNextRTTWorkRequired: Assertion `pprev' failed.
Aborted (core dumped)
[280/509] Running secp256k1 test suite
PASSED: secp256k1 test suite
[468/509] Running pow test suite
PASSED: pow test suite
[482/509] Running seeder test suite
PASSED: seeder test suite
[489/509] Running avalanche test suite
PASSED: avalanche test suite
[494/509] Running bitcoin-qt test suite
PASSED: bitcoin-qt test suite
[506/509] Running utility command for check-bitcoin-coins_tests
ninja: build stopped: cannot make progress due to previous errors.
Build build-debug failed with exit code 1

Remove redundant Assume, unbreak bitcoin-chainstate build

bytesofman added inline comments.
src/node/miner.cpp
49 ↗(On Diff #48266)

what is getAdjustedTime and how does it differ from what is currently being used?

src/policy/block/rtt.cpp
93 ↗(On Diff #48266)

could be read as "min" === "minute"

src/test/arith_uint256_tests.cpp
670 ↗(On Diff #48266)

is this testing cpp...?

seems like this test is asserting a definition but I suppose it's safer to include than to not.

src/test/rtt_tests.cpp
73 ↗(On Diff #48266)

prob me just not understanding BOOST_CHECK or cpp syntax:

prevRttWork is defined before this for loop, and nextWork is calculated for each new value of t in this loop

is this confirming that nextWork is always the same as the previously calculated prevRttWork, i.e. it is not changing with t?

Fabien marked an inline comment as done.Jun 17 2024, 18:42
Fabien added inline comments.
src/node/miner.cpp
49 ↗(On Diff #48266)

The adjusted time is the node local time plus/minus an offset which is (iirc) the median of the time difference against the other peers, if you have enough peers. So if your node local time is off it could be adjusted by looking at the time error wrt connected peers.

I'm not sure whether this is relevant here. Also note that the adjusted time is actually bugged (see timedata.cpp).

src/test/arith_uint256_tests.cpp
670 ↗(On Diff #48266)

It's testing the ability of the class arith_uint256 to represent a double, see the roundtrip conversion to/from double. This only works for some values though since a double cannot represent any integer value without losing precision, which is why there is this comment: it's about the limit of the loop.

src/test/rtt_tests.cpp
73 ↗(On Diff #48266)

Each loop iteration compute the next difficulty target, check it is greater than the previous one, and copies the new value as the previous in preparation for the next iteration.

The BOOST_CHECK(nBitsLE(prevRttWork, *nextWork)); asserts that the previous real time target is <= than the new one (if the target increases the difficulty decreases: it becomes easier to find a hash that is below this target).

The overall loop checks the target increases (so difficulty decreases) over time t.

Update confusing comment min vs minimum

Concept ACK. The code and tests make sense. I need to do another review pass focusing just on the math.

roqqit added inline comments.
src/test/rtt_tests.cpp
76–79 ↗(On Diff #48279)

Comment says never. Should check across various timestamps for sanity like: now + [0, 1, 100, 1000, 3600, 24 * 60 * 60]

Improve test as per feedback.
Also include another loop iteration on the arith256 test which was missing for no reason.

Simplify by using the block template target instead of the header bits.

Remaining to do:

  • Add more info to the block template so the diff can be re-computed by the mining pool software
  • Handle longpoll

Tail of the build log:

/work /work/abc-ci-builds/lint-circular-dependencies
A new circular dependency in the form of "policy/block/rtt -> validation -> policy/block/rtt" appears to have been introduced.

/work/abc-ci-builds/lint-circular-dependencies
Build lint-circular-dependencies failed with exit code 1

fix circular dependency, minor fixes, return computation data in the block template

Rework the algo to use integer arithmetics.
Rebase on D16593 and D16594, remove experimental logging + minor test fixes.

Missed one experimental log reference

Cleanup miner.cpp and miner_tests.cpp

Include a test that ensures required work does not drift or change unexpectedly over time. ie given same previous target and time diff, we should expect timestamps a year or more in the future to give the same RTT as today's timestamps. By picking a large delta of over a year, we gain confidence that the next 6-month upgrade does not break the RTT policy.

src/test/rtt_tests.cpp
70 ↗(On Diff #49060)

This and similar checks should be strictly less than, otherwise this isn't testing what it says it is: that the target increases over time.

78 ↗(On Diff #49060)

include boundary t = 599

146–150 ↗(On Diff #49060)

Use the same loops testing values of t that the previous test uses.

src/test/rtt_tests.cpp
78 ↗(On Diff #49060)

it's not the boundary, which happens at ~430s. I use 600s below because it should always be the case that the diff is "back to normal" at the expected block time interval.
Not including the boundary allows for changing the k value (and so the response curve) without having to change the test, but at this stage it's certainly OK to assume the value won't change.

Fabien edited the summary of this revision. (Show Details)

Feedback