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 18845
Build 37480: Build Diff	build-without-wallet · build-diff · lint-circular-dependencies · build-debug · build-clang · build-clang-tidy
Build 37479: 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 ↗	(On Diff #31404)	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 ↗	(On Diff #31404)	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 ↗	(On Diff #31404)	That is a minimal 1ms ? Can the loop run in under 1ms ?
418 ↗	(On Diff #31404)	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 ↗	(On Diff #31404)	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 ↗	(On Diff #31404)	You are right.
418 ↗	(On Diff #31404)	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.