Details

Reviewers

Group Reviewers

Restricted Owners Package	(Owns No Changed Paths)
Restricted Project

Commits

rABC4223eb71c7ea: Make the seeder dump interval settable by commandline argument

Summary

The current scheme for delays between seeder dumps is not useful for some situations, namely:

First dump takes too long (100 seconds) for automated or manual testing
Interval backoff is not useful under any circumstance that I can think of
Final interval is not intuitive (53.333 minutes) so making a cron job that picks up the latest dump is not obvious

This patch fixes these issues by introducing the -dumpinterval commandline argument.
Default dumpinterval is changed to 1 hour.

Test Plan

ninja bitcoin-seeder check-seeder

Start seeder with default dumpinterval:

date
./src/seeder/bitcoin-seeder -host=seeder.status.cash -ns=localhost -mbox=thonkle@protonmail.com -port=5555

1 hour later, verify dnsseed.dump is 1 hour after the above timestamp:

ls -al dnsseed.dump

Verify quick dumpinterval works (dnsseed.dump timestamps should update every 1 second):

./src/seeder/bitcoin-seeder -host=seeder.status.cash -ns=localhost -mbox=thonkle@protonmail.com -port=5555 -dumpinterval=1
ls -al --full-time dnsseed.dump && sleep 1
ls -al --full-time dnsseed.dump && sleep 1
ls -al --full-time dnsseed.dump && sleep 1

Diff Detail

Repository

rABC Bitcoin ABC

Branch

dump-interval

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 17636
Build 35098: Build Diff	build-without-wallet · build-diff · lint-circular-dependencies · build-clang-tidy · build-clang · build-debug
Build 35097: arc lint + arc unit

Event Timeline

thonkle created this revision.Dec 10 2021, 22:06

Owners added a reviewer: Restricted Owners Package.Dec 10 2021, 22:06

Herald added a reviewer: Restricted Project. · View Herald TranscriptDec 10 2021, 22:06

thonkle requested review of this revision.Dec 10 2021, 22:06

Harbormaster completed remote builds in B17615: Diff 31364.Dec 10 2021, 22:21

Build Bitcoin ABC Diffs / Diff Testing (build-diff) failed.

Tail of the build log:

[0m[0;32mrpc_uptime.py                             | ✓ Passed  | 1 s
[0m[0;32mrpc_users.py                              | ✓ Passed  | 5 s
[0m[0;32mrpc_whitelist.py                          | ✓ Passed  | 1 s
[0m[0;32mtool_wallet.py                            | ✓ Passed  | 4 s
[0m[0;32mwallet_abandonconflict.py                 | ✓ Passed  | 12 s
[0m[0;32mwallet_address_types.py                   | ✓ Passed  | 15 s
[0m[0;32mwallet_avoidreuse.py                      | ✓ Passed  | 4 s
[0m[0;32mwallet_avoidreuse.py --descriptors        | ✓ Passed  | 4 s
[0m[0;32mwallet_backup.py                          | ✓ Passed  | 25 s
[0m[0;32mwallet_balance.py                         | ✓ Passed  | 23 s
[0m[0;32mwallet_basic.py                           | ✓ Passed  | 19 s
[0m[0;32mwallet_coinbase_category.py               | ✓ Passed  | 1 s
[0m[0;32mwallet_create_tx.py                       | ✓ Passed  | 6 s
[0m[0;32mwallet_createwallet.py                    | ✓ Passed  | 2 s
[0m[0;32mwallet_createwallet.py --usecli           | ✓ Passed  | 3 s
[0m[0;32mwallet_descriptor.py                      | ✓ Passed  | 7 s
[0m[0;32mwallet_disable.py                         | ✓ Passed  | 0 s
[0m[0;32mwallet_dump.py                            | ✓ Passed  | 5 s
[0m[0;32mwallet_encryption.py                      | ✓ Passed  | 5 s
[0m[0;32mwallet_encryption.py --descriptors        | ✓ Passed  | 5 s
[0m[0;32mwallet_hd.py                              | ✓ Passed  | 6 s
[0m[0;32mwallet_hd.py --descriptors                | ✓ Passed  | 4 s
[0m[0;32mwallet_import_rescan.py                   | ✓ Passed  | 4 s
[0m[0;32mwallet_import_with_label.py               | ✓ Passed  | 1 s
[0m[0;32mwallet_importdescriptors.py               | ✓ Passed  | 5 s
[0m[0;32mwallet_importmulti.py                     | ✓ Passed  | 3 s
[0m[0;32mwallet_importprunedfunds.py               | ✓ Passed  | 2 s
[0m[0;32mwallet_importprunedfunds.py --descriptors | ✓ Passed  | 2 s
[0m[0;32mwallet_keypool.py                         | ✓ Passed  | 3 s
[0m[0;32mwallet_keypool_topup.py                   | ✓ Passed  | 3 s
[0m[0;32mwallet_keypool_topup.py --descriptors     | ✓ Passed  | 3 s
[0m[0;32mwallet_labels.py                          | ✓ Passed  | 1 s
[0m[0;32mwallet_labels.py --descriptors            | ✓ Passed  | 1 s
[0m[0;32mwallet_listreceivedby.py                  | ✓ Passed  | 18 s
[0m[0;32mwallet_listsinceblock.py                  | ✓ Passed  | 3 s
[0m[0;32mwallet_listtransactions.py                | ✓ Passed  | 23 s
[0m[0;32mwallet_listtransactions.py --descriptors  | ✓ Passed  | 10 s
[0m[0;32mwallet_multiwallet.py                     | ✓ Passed  | 41 s
[0m[0;32mwallet_multiwallet.py --usecli            | ✓ Passed  | 15 s
[0m[0;32mwallet_reorgsrestore.py                   | ✓ Passed  | 3 s
[0m[0;32mwallet_resendwallettransactions.py        | ✓ Passed  | 9 s
[0m[0;32mwallet_send.py                            | ✓ Passed  | 10 s
[0m[0;32mwallet_startup.py                         | ✓ Passed  | 2 s
[0m[0;32mwallet_txn_clone.py                       | ✓ Passed  | 2 s
[0m[0;32mwallet_txn_clone.py --mineblock           | ✓ Passed  | 3 s
[0m[0;32mwallet_txn_doublespend.py                 | ✓ Passed  | 1 s
[0m[0;32mwallet_txn_doublespend.py --mineblock     | ✓ Passed  | 4 s
[0m[0;32mwallet_watchonly.py                       | ✓ Passed  | 1 s
[0m[0;32mwallet_watchonly.py --usecli              | ✓ Passed  | 1 s
[0m[1m
ALL                                       | ✓ Passed  | 1055 s (accumulated) 
[0mRuntime: 219 s

----------------------------------------------------------------------
Ran 10 tests in 0.092s

OK

ninja: build stopped: cannot make progress due to previous errors.
Build build-diff failed with exit code 1

With the previous behavior I had to wait for 100s before I can check that the seeder is dumping data properly. This was inconvenient but now it's worst because I have to wait for 1h.
I don't see any reason for the intermediate intervals but the first one is useful imo.

src/seeder/main.cpp
412 ↗	(On Diff #31364)	You need to check (assert ?) the pointer is not null first. Also it can be unsigned, or even better std::chrono::seconds.
414 ↗	(On Diff #31364)	There is no bound checking on the dump interval. What happens with 0 ? or -1 ?

This revision now requires changes to proceed.Dec 11 2021, 12:49

Add a dump soon after startup (up to 10 sec)
Assert thread data pointer
Add bounds checking
Use std::chrono

Harbormaster completed remote builds in B17636: Diff 31404.Dec 14 2021, 23:54

Fabien requested changes to this revision.Dec 15 2021, 13:05

Fabien added inline comments.

src/seeder/main.cpp
95	This is where you need to check the bounds, so you can return an explicit error message instead of silently overriding the limit parameters (which are not documented).
413	What's the point of building a std::chrono::milliseconds here to cast it to int everywhere after ? It should already be a std::chrono::duration at this point
414	That is a minimal 1ms ? Can the loop run in under 1ms ?
418	You probably want to create a version of Sleep that takes a std::chrono::duration

This revision now requires changes to proceed.Dec 15 2021, 13:05

thonkle added inline comments.Dec 15 2021, 23:22

src/seeder/main.cpp
95	The precendent for bounds checking on these arguments was poor. I will author a separate diff to set one and then revisit this patch to match.
413	You are right.
418	This will need its own diff. The seeder should be using the same functions in `src/util/time.*` instead of its own. Hence, I did not want to go there in this patch.