Page MenuHomePhabricator

Add a block policy to reject turbo blocks

Authored by Fabien on Mon, Jun 17, 14:50.
This is a draft revision that has not yet been submitted for review.


Group Reviewers
Restricted Project

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

This needs to be split into pieces, and ideally the double computation should be removed for int256 computation. I also left some debug prints under a new experimental category that should eventually get removed.

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/ 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:            | ○ Skipped | 0 s                  | ○ Skipped | 0 s          | ○ Skipped | 0 s                    | ○ Skipped | 0 s                           | ○ Skipped | 0 s                        | ○ Skipped | 0 s                       | ○ Skipped | 0 s             | ○ Skipped | 0 s                      | ○ Skipped | 0 s                  | ○ Skipped | 0 s             | ○ Skipped | 0 s              | ○ Skipped | 0 s              | ○ Skipped | 0 s            | ○ Skipped | 0 s                  | ○ Skipped | 0 s                              | ○ Skipped | 0 s                | ○ Skipped | 0 s                              | ○ Skipped | 0 s                     | ○ Skipped | 0 s                         | ○ Skipped | 0 s                       | ○ Skipped | 0 s              | ○ Skipped | 0 s            | ○ Skipped | 0 s                      | ○ Skipped | 0 s                | ○ Skipped | 0 s               | ○ Skipped | 0 s

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

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

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

[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/ /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.
49 ↗(On Diff #48266)

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

93 ↗(On Diff #48266)

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

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.

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.Mon, Jun 17, 18:42
Fabien added inline comments.
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).

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.

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.
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.