Page MenuHomePhabricator
Differential D16500

[seeder] Mark nodes as unreliable until checkpoint verified
ClosedPublic
Actions

Authored by PiRK on Jul 18 2024, 10:49.

Details

Reviewers
Fabien
Group Reviewers
Restricted Project
Commits
rABCdf673a6df8aa: [seeder] Mark nodes as unreliable until checkpoint verified
Summary

This improves on D16465 by waiting for the response to GETHEADERS before considering a node to be reliable.
The GETADDR and GETHEADERS logic is reworked so that the messages are now send independantly of each other (GETADDR is only sent once a day for each node). There is still an issue caused by the fact that if the ADDR message arrives the node is done, and as such it can miss out on the HEADERS and incorrectly mark nodes as bad. This issue is fixed in a separate diff.

Upon first start of a seeder after this commit, all nodes from the existing are considered to have a verified checkpoint (until proven otherwise). This is to avoid clearing the entire db immediately and then have to repopulate it.

This is a partial backport of bchn#1820
https://gitlab.com/bitcoin-cash-node/bitcoin-cash-node/-/merge_requests/1820/diffs?commit_id=4c8604800ba9ea67c61dd8c7c426d260004d853e
https://gitlab.com/bitcoin-cash-node/bitcoin-cash-node/-/merge_requests/1820/diffs?commit_id=878d33146204d87b05697c1ada093013b1ff36ae
https://gitlab.com/bitcoin-cash-node/bitcoin-cash-node/-/merge_requests/1820/diffs?commit_id=02ebf3679f9c1db8e88b966c028656ebc7bd4619
https://gitlab.com/bitcoin-cash-node/bitcoin-cash-node/-/merge_requests/1820/diffs?commit_id=61dca262b3b411a4ec906c838e5587517d7fcc5d
https://gitlab.com/bitcoin-cash-node/bitcoin-cash-node/-/merge_requests/1820/diffs?commit_id=024473ee09dcc9371cd73b51bea74c69a7692949
https://gitlab.com/bitcoin-cash-node/bitcoin-cash-node/-/merge_requests/1820/diffs?commit_id=e702140c2afa957d83351522586a23a6cb73d388

Depends on D16499

Test Plan

run the seeder before and after this change, confirm that we eventually get only Bitcoin ABC nodes (after waiting for long enough)

ninja bitcoin-seeder check-seeder
src/seeder/bitcoin-seeder -dumpinterval=60

After a very long time:

awk '{ if ($2 == 1) { print } }' dnsseed.dump

Diff Detail

Event Timeline

PiRK created this revision.Jul 18 2024, 10:49
Herald added a reviewer: Restricted Project. · View Herald TranscriptJul 18 2024, 10:49
teamcity edited the summary of this revision. (Show Details)Jul 18 2024, 10:49
PiRK added inline comments.Jul 18 2024, 10:53
src/seeder/bitcoin.cpp
154–155

comment

159–160

comment

src/seeder/main.cpp
59

revert

Harbormaster completed remote builds in B29624: Diff 48722.Jul 18 2024, 11:30
teamcity added a comment.Jul 18 2024, 11:31
Build Bitcoin ABC Diffs / Diff Testing (build-without-wallet) failed.

Tail of the build log:

wallet_labels.py --descriptors             | ○ Skipped | 0 s
wallet_listreceivedby.py                   | ○ Skipped | 0 s
wallet_listsinceblock.py                   | ○ Skipped | 0 s
wallet_listsinceblock.py --descriptors     | ○ Skipped | 0 s
wallet_listtransactions.py                 | ○ Skipped | 0 s
wallet_listtransactions.py --descriptors   | ○ Skipped | 0 s
wallet_multiwallet.py                      | ○ Skipped | 0 s
wallet_multiwallet.py --descriptors        | ○ Skipped | 0 s
wallet_multiwallet.py --usecli             | ○ Skipped | 0 s
wallet_reorgsrestore.py                    | ○ Skipped | 0 s
wallet_resendwallettransactions.py         | ○ Skipped | 0 s
wallet_send.py                             | ○ Skipped | 0 s
wallet_startup.py                          | ○ Skipped | 0 s
wallet_timelock.py                         | ○ Skipped | 0 s
wallet_txn_clone.py                        | ○ Skipped | 0 s
wallet_txn_clone.py --mineblock            | ○ Skipped | 0 s
wallet_txn_doublespend.py                  | ○ Skipped | 0 s
wallet_txn_doublespend.py --mineblock      | ○ Skipped | 0 s
wallet_watchonly.py                        | ○ Skipped | 0 s
wallet_watchonly.py --usecli               | ○ Skipped | 0 s

ALL                                        | ✓ Passed  | 880 s (accumulated) 
Runtime: 177 s

[32/474] Test Bitcoin RPC authentication...
...
----------------------------------------------------------------------
Ran 3 tests in 0.005s

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

OK
[136/474] Building CXX object src/test/CMakeFiles/test_bitcoin.dir/script_tests.cpp.o
In file included from /usr/include/boost/test/unit_test.hpp:19,
                 from ../../src/test/script_tests.cpp:31:
../../src/test/script_tests.cpp: In member function ‘void script_tests::script_build::test_method()’:
../../src/test/script_tests.cpp:541:22: note: variable tracking size limit exceeded with ‘-fvar-tracking-assignments’, retrying without
  541 | BOOST_AUTO_TEST_CASE(script_build) {
      |                      ^~~~~~~~~~~~
[201/474] Running bitcoin-qt test suite
PASSED: bitcoin-qt test suite
[438/474] Running avalanche test suite
PASSED: avalanche test suite
[440/474] Running pow test suite
PASSED: pow test suite
[442/474] seeder: testing db_tests
FAILED: src/seeder/test/CMakeFiles/check-seeder-db_tests 
cd /work/abc-ci-builds/build-without-wallet/src/seeder/test && /usr/bin/cmake -E make_directory /work/abc-ci-builds/build-without-wallet/test/junit && /usr/bin/cmake -E make_directory /work/abc-ci-builds/build-without-wallet/test/log && /usr/bin/cmake -E env /work/cmake/utils/log-and-print-on-failure.sh /work/abc-ci-builds/build-without-wallet/test/log/seeder-db_tests.log /work/abc-ci-builds/build-without-wallet/src/seeder/test/test-seeder --run_test=db_tests --logger=HRF,message:JUNIT,message,seeder-db_tests.xml --catch_system_errors=no
Running 1 test case...
../../src/seeder/test/db_tests.cpp(60): error: in "db_tests/seederaddrinfo_test": check info.IsReliable() has failed

*** 1 failure is detected in the test module "Seeder Test Suite"
[471/474] Running bitcoin test suite
PASSED: bitcoin test suite
ninja: build stopped: cannot make progress due to previous errors.
Build build-without-wallet failed with exit code 1
teamcity added a comment.Jul 18 2024, 11:38
Build Bitcoin ABC Diffs / Diff Testing (build-debug) failed.

Tail of the build log:

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  | 2924 s (accumulated) 
Runtime: 586 s

[171/514] Test Bitcoin RPC authentication...
...
----------------------------------------------------------------------
Ran 3 tests in 0.005s

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

OK
[215/514] seeder: testing db_tests
FAILED: src/seeder/test/CMakeFiles/check-seeder-db_tests 
cd /work/abc-ci-builds/build-debug/src/seeder/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/seeder-db_tests.log /work/abc-ci-builds/build-debug/src/seeder/test/test-seeder --run_test=db_tests --logger=HRF,message:JUNIT,message,seeder-db_tests.xml --catch_system_errors=no
Running 1 test case...
../../src/seeder/test/db_tests.cpp(60): error: in "db_tests/seederaddrinfo_test": check info.IsReliable() has failed
../../src/seeder/test/db_tests.cpp(66): error: in "db_tests/seederaddrinfo_test": check info.IsReliable() has failed

*** 2 failures are detected in the test module "Seeder Test Suite"
[283/514] Running secp256k1 test suite
PASSED: secp256k1 test suite
[482/514] Running pow test suite
PASSED: pow test suite
[498/514] Running avalanche test suite
PASSED: avalanche test suite
[500/514] Running bitcoin-qt test suite
PASSED: bitcoin-qt test suite
[511/514] Running bitcoin test suite
PASSED: bitcoin test suite
ninja: build stopped: cannot make progress due to previous errors.
Build build-debug failed with exit code 1
teamcity added a comment.Jul 18 2024, 11:49
Build Bitcoin ABC Diffs / Diff Testing (build-diff) failed.

Tail of the build log:

wallet_multiwallet.py --usecli             | ✓ Passed  | 25 s
wallet_reorgsrestore.py                    | ✓ Passed  | 7 s
wallet_resendwallettransactions.py         | ✓ Passed  | 11 s
wallet_send.py                             | ✓ Passed  | 16 s
wallet_startup.py                          | ✓ Passed  | 6 s
wallet_timelock.py                         | ✓ Passed  | 2 s
wallet_txn_clone.py                        | ✓ Passed  | 3 s
wallet_txn_clone.py --mineblock            | ✓ Passed  | 6 s
wallet_txn_doublespend.py                  | ✓ Passed  | 4 s
wallet_txn_doublespend.py --mineblock      | ✓ Passed  | 5 s
wallet_watchonly.py                        | ✓ Passed  | 3 s
wallet_watchonly.py --usecli               | ✓ Passed  | 3 s
chronik_avalanche.py                       | ○ Skipped | 0 s
chronik_block.py                           | ○ Skipped | 0 s
chronik_block_info.py                      | ○ Skipped | 0 s
chronik_block_txs.py                       | ○ Skipped | 0 s
chronik_blockchain_info.py                 | ○ Skipped | 0 s
chronik_blocks.py                          | ○ Skipped | 0 s
chronik_chronik_info.py                    | ○ Skipped | 0 s
chronik_cors.py                            | ○ Skipped | 0 s
chronik_disable_token_index.py             | ○ Skipped | 0 s
chronik_disallow_prune.py                  | ○ Skipped | 0 s
chronik_lokad_id_group.py                  | ○ Skipped | 0 s
chronik_mempool_conflicts.py               | ○ Skipped | 0 s
chronik_pause.py                           | ○ Skipped | 0 s
chronik_plugins_setup.py                   | ○ Skipped | 0 s
chronik_raw_tx.py                          | ○ Skipped | 0 s
chronik_resync.py                          | ○ Skipped | 0 s
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  | 2245 s (accumulated) 
Runtime: 450 s

ninja: build stopped: cannot make progress due to previous errors.
Build build-diff failed with exit code 1
PiRK updated this revision to Diff 48724.Jul 18 2024, 12:27

improve tests, remove debugging prints

PiRK edited the test plan for this revision. (Show Details)Jul 18 2024, 12:30
Harbormaster completed remote builds in B29626: Diff 48724.Jul 18 2024, 12:51
PiRK updated this revision to Diff 50086.Mon, Oct 14, 18:58
PiRK edited the summary of this revision. (Show Details)
PiRK edited the test plan for this revision. (Show Details)

squash a bunch of additional commits from the same MR to have a complete feature

PiRK added a child revision: D16908: [seeder] Fix such that seeder node awaits both headers and addr messages.Mon, Oct 14, 19:01
Harbormaster completed remote builds in B30590: Diff 50086.Mon, Oct 14, 19:17
PiRK published this revision for review.Tue, Oct 15, 05:08
PiRK retitled this revision from Seeder: Mark nodes as unreliable until checkpoint verified to [seeder] Mark nodes as unreliable until checkpoint verified.
Fabien requested changes to this revision.Tue, Oct 15, 07:45
Fabien added a subscriber: Fabien.
Fabien added inline comments.
src/seeder/db.h
29 ↗(On Diff #50086)

I think this is not accurate, you also need to check the height of the last checkpoint is > 0.
I don't think the nodes will send the genesis header, so if you think regtest where the only checkpoint is the genesis block this would not work.

This revision now requires changes to proceed.Tue, Oct 15, 07:45
PiRK added inline comments.Tue, Oct 15, 07:55
src/seeder/db.h
29 ↗(On Diff #50086)

OK for the additional checkpoint height test. I was thinking more about the first couple of weeks after we start a new testnet (before it gets a checkpoint), or maybe Signet if we ever backport it.

PiRK updated this revision to Diff 50110.Tue, Oct 15, 14:16

check last checkpoint height to decide whether checkpoints should be validated
This requires a unit test to now use the MainNet params.

PiRK updated this revision to Diff 50111.Tue, Oct 15, 14:18

use GetRequireHeight in HasCheckpoint

Harbormaster completed remote builds in B30605: Diff 50110.Tue, Oct 15, 14:35
Harbormaster completed remote builds in B30606: Diff 50111.Tue, Oct 15, 14:40
Fabien accepted this revision.Tue, Oct 15, 19:22
This revision is now accepted and ready to land.Tue, Oct 15, 19:22
Closed by commit rABCdf673a6df8aa: [seeder] Mark nodes as unreliable until checkpoint verified (authored by PiRK). · Explain WhyTue, Oct 15, 20:08
This revision was automatically updated to reflect the committed changes.
PiRK added a commit: rABCdf673a6df8aa: [seeder] Mark nodes as unreliable until checkpoint verified.

Revision Contents
Changeset List

PathSize
src/
seeder/
3 lines
25 lines
17 lines
3 lines
3 lines
test/
14 lines

Diff 48722

View Options

src/seeder/bitcoin.h

Loading...
View Options

src/seeder/bitcoin.cpp

Loading...
View Options

src/seeder/db.h

Loading...
View Options

src/seeder/db.cpp

Loading...
View Options

src/seeder/main.cpp

Loading...
View Options

src/seeder/test/p2p_messaging_tests.cpp

Loading...