Page MenuHomePhabricator

Make the seeder dump interval settable by commandline argument
ClosedPublic

Authored by thonkle on Dec 10 2021, 22:06.

Details

Reviewers
Fabien
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

Event Timeline

Owners added a reviewer: Restricted Owners Package.Dec 10 2021, 22:06
thonkle requested review of this revision.Dec 10 2021, 22:06

Tail of the build log:

rpc_uptime.py                             | ✓ Passed  | 1 s
rpc_users.py                              | ✓ Passed  | 5 s
rpc_whitelist.py                          | ✓ Passed  | 1 s
tool_wallet.py                            | ✓ Passed  | 4 s
wallet_abandonconflict.py                 | ✓ Passed  | 12 s
wallet_address_types.py                   | ✓ Passed  | 15 s
wallet_avoidreuse.py                      | ✓ Passed  | 4 s
wallet_avoidreuse.py --descriptors        | ✓ Passed  | 4 s
wallet_backup.py                          | ✓ Passed  | 25 s
wallet_balance.py                         | ✓ Passed  | 23 s
wallet_basic.py                           | ✓ Passed  | 19 s
wallet_coinbase_category.py               | ✓ Passed  | 1 s
wallet_create_tx.py                       | ✓ Passed  | 6 s
wallet_createwallet.py                    | ✓ Passed  | 2 s
wallet_createwallet.py --usecli           | ✓ Passed  | 3 s
wallet_descriptor.py                      | ✓ Passed  | 7 s
wallet_disable.py                         | ✓ Passed  | 0 s
wallet_dump.py                            | ✓ Passed  | 5 s
wallet_encryption.py                      | ✓ Passed  | 5 s
wallet_encryption.py --descriptors        | ✓ Passed  | 5 s
wallet_hd.py                              | ✓ Passed  | 6 s
wallet_hd.py --descriptors                | ✓ Passed  | 4 s
wallet_import_rescan.py                   | ✓ Passed  | 4 s
wallet_import_with_label.py               | ✓ Passed  | 1 s
wallet_importdescriptors.py               | ✓ Passed  | 5 s
wallet_importmulti.py                     | ✓ Passed  | 3 s
wallet_importprunedfunds.py               | ✓ Passed  | 2 s
wallet_importprunedfunds.py --descriptors | ✓ Passed  | 2 s
wallet_keypool.py                         | ✓ Passed  | 3 s
wallet_keypool_topup.py                   | ✓ Passed  | 3 s
wallet_keypool_topup.py --descriptors     | ✓ Passed  | 3 s
wallet_labels.py                          | ✓ Passed  | 1 s
wallet_labels.py --descriptors            | ✓ Passed  | 1 s
wallet_listreceivedby.py                  | ✓ Passed  | 18 s
wallet_listsinceblock.py                  | ✓ Passed  | 3 s
wallet_listtransactions.py                | ✓ Passed  | 23 s
wallet_listtransactions.py --descriptors  | ✓ Passed  | 10 s
wallet_multiwallet.py                     | ✓ Passed  | 41 s
wallet_multiwallet.py --usecli            | ✓ Passed  | 15 s
wallet_reorgsrestore.py                   | ✓ Passed  | 3 s
wallet_resendwallettransactions.py        | ✓ Passed  | 9 s
wallet_send.py                            | ✓ Passed  | 10 s
wallet_startup.py                         | ✓ Passed  | 2 s
wallet_txn_clone.py                       | ✓ Passed  | 2 s
wallet_txn_clone.py --mineblock           | ✓ Passed  | 3 s
wallet_txn_doublespend.py                 | ✓ Passed  | 1 s
wallet_txn_doublespend.py --mineblock     | ✓ Passed  | 4 s
wallet_watchonly.py                       | ✓ Passed  | 1 s
wallet_watchonly.py --usecli              | ✓ Passed  | 1 s

ALL                                       | ✓ Passed  | 1055 s (accumulated) 
Runtime: 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
Fabien requested changes to this revision.Dec 11 2021, 12:49
Fabien added a subscriber: Fabien.

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

Rebase on patches that address feedback

Fabien added inline comments.
src/seeder/main.cpp
243 ↗(On Diff #33243)

Style nit: you can use auto to avoid redundancy

This revision is now accepted and ready to land.Apr 19 2022, 07:19