GitLab

Add Bitcoin Unlimited operated seeder for testnet3

See merge request !1239

Summary
---

In preparation for a next release, we should update the seed list.
This MR was prepared by following the instructions in
`contrib/seeds/README.md`.

The source `dnsseeds.dump` used was from my server at `bchseed.c3-soft.com`
which has been running for >30 days.

Also in this MR:

- Updated the `makeseeds.py` regex to:
  - No longer accept BCHN 22.x.x for the seeder (22.x.x by now is no longer really
  conforming to latest relay rules).
  - Updated the kth-bch regex for latest knuth
  - Updatede the dnsdump.dump.test file to reflect the changed regexes
    for testing.

Test Plan
---

- `ninja all check`
- Start up a full node **on mainnet**, having made sure to delete its
  peers.dat file first.
  - To ensure that this node uses the seeds and not the DNS seeder, you
    should override `/etc/hosts` on your system to point all the hard-coded
    seeders to 127.0.0.1, so that the node is forced to use the hard-coded seeds
    from this MR, rather than the seeder.
  - Example `/etc/hosts` below:

```
127.0.0.1 seed-bch.bitcoinforks.org
127.0.0.1 btccash-seeder.bitcoinunlimited.info
127.0.0.1 seed.bchd.cash
127.0.0.1 seed.bch.loping.net
127.0.0.1 dnsseed.electroncash.de
127.0.0.1 bchseed.c3-soft.com

::1 seed-bch.bitcoinforks.org
::1 btccash-seeder.bitcoinunlimited.info
::1 seed.bchd.cash
::1 seed.bch.loping.net
::1 dnsseed.electroncash.de
::1 bchseed.c3-soft.com
```

Summary
---

This data is not used for anything critical other than the Bitcoin-Qt UI
to display estimated progress info, and also various RPCs that provide information only.

It has been updated for the 3 networks since we will do a release soon.

The tx rate estimate was calculated for by just taking the
average rate since the last time we did this calculation.

Test Plan
---

- `ninja all check`
- Verify that the changed block height and timestamps match.

New Crowdin updates

See merge request !1212

This tests Consensus::CheckTxInputs. This bench will allow us to optimize this
particular call.

[doc] Remove personal disclosure contact

See merge request !1227

Summary
---

When working on other code, I noticed that on Linux on g++-8, I was
getting the following compiler warning when compiling txmempool.cpp:

    ../src/txmempool.cpp: In lambda function:
    ../src/txmempool.cpp:903:20: warning: declaration of ‘const TxId txId’ shadows a parameter [-Wshadow]
             const TxId txId{txit->GetTx().GetId()};
                        ^~~~
    ../src/txmempool.cpp:893:53: note: shadowed declaration is here
      auto CTxMemPool::recursiveDSProofSearch(const TxId &txId, DspDescendants *desc, double *score) const
                                             ~~~~~~~~~~~~^~~~

This commit fixes the warning.

Test Plan
---

No real changes to this codebase occur here. But:

- `ninja all check check-functional` should do it.

[build] Fix fuzz-libfuzzer undefined references for IsPayToPubKeyHash / IsPayToScriptHash

See merge request !1224

Summary
---

As discussed in Slack, perhaps it would be beneficial to optionally skip
the slow `TestBlockValidity` checks when generating a block template.
These checks dominate block template creation for large blocks, and
in some cases perhaps advanced users (miners, etc) may want to skip these
checks.  Note that it is a bug in this codebase if we *ever* generate
an invalid block template anyway, so the checks are largely redundant.

Additoinally, while working on this code, I noticed:

- RPC help for GBT(L) is incomplete. I added the missing `"longpollid"`
documentation.
- RPC help for GBT(L) had some redundant pieces. I put the duplicated
stuff into a common static function `getGBTArgs()`.  This makes it
easier to maintain the arg code for those two very strongly related functions.
- I noticed it's annoying when testing that block templates are cached. Perhaps we
want to skip caching, especially when testing.  I added the optional
RPC argument to the `template_request` called `"ignorecache"` which, if true,
always skips the cached block template and generates a new one.

Summary of Changes
---

- New arg: `-gbtcheckvalidity` to control the default setting for gbt(l).
  If false, we skip the validity checks when generating a block template
  (default: true)
- Added help doc for `"longpollid"` `template_request` argument in RPC for
  gbt(l)
- Added optional `"ignorecache"` `template_request` argument (default:
  false) to skip the cached block template unconditionally (useful for
  testing).
- Added optional `"checkvalidity"` `template_request` argument (default:
  whatever `-gbtcheckvalidity` was set to on CLI), which controls the
  validity checker on a per-template basis.
- Refactored the help for `getblocktemplate` and `getblocktemplatelight`
  into a common function.
- In `checkvalidity=false` mode, also skip the redundant and wasteful
  calculation of block size for the 6th time just to print it to debug log
  (and instead use the estimated worst-case size we already know).

Test Plan
---

1. Review
2. `ninja all check`
3. `test/functional/test_runner.py bchn-rpc-gbt-checkvalidity-ignorecache`

Summary
---

This new method is intended to address and close #307.
The new RPC takes a txid as its only argument and
returns a number from `0.0` to `1.0`.

- `1.0` indicates no dsproofs exist for this tx or for any of its ancestors,
  but that it and all in-mempool ancestors **can** have a dsproof,
  so confidence should be high that the tx is ok.
- `0.0` indicates that either this tx or one of its ancestors has a dsproof,
  or that it or one of its in-mempool ancestors can never have a proof
  (not P2PKH) -- so confidence should be low.
- `0.25` is returned in the case where the tx has so many mempool ancestors
  that no conclusive determination could be made (but the ones that were
  checked are ok).
- If `txid` doesn't exist in the mempool, the usual `JSONRPCError` is thrown
  (similar to how other RPCs work).

Code Changed
---

- Refactored out the code from `compressor.cpp` that checked for p2pkh and p2sh
  and ensured this code (which was somewhat duplicated) all ...

Summary
---

This MR adds dsproof saving to disk on node shutdown, and reload
from disk on node startup.

Motivation: While I was doing some testing and developing I noticed that
dsproofs get lost across restarts.  Sometimes node admins need to bounce
nodes and it would be nice if along with the mempool being saved/reloaded,
the dsproof state were also preserved across restarts.

This will become increasingly important in the future when wallets that
connect to servers such as Fulcrum start depending on seeing and
querying the dsproof state of particular transactions.

Description of Change
---

The dsproofs get saved to `datadir/dsproofs.dat` in a mechanism similar to
how the mempool is persisted to `datadir/mempool.dat`.  This is done once on
node shutdown.

On node restart, this file is loaded before the mempool is loaded and the
proofs are put into dsproof storage as "orphans".

Should the associated tx for the proof show up in the mempool later when
the mempool is loaded, the proofs will be re-associated with their
transactions.

Should the associated tx not ever show up or should it get immediately
confirmed by new blocks (after a longer period of being offline), the proofs
are never used and will remain as orphans, to be reaped within 60 seconds
(normal orphan reaping timeout).

The existing CLI arg `-persistmempool` is consulted when deciding whether
to save the `dsproofs.dat` file (alongside `mempool.dat`).  This is because
the two files really go together data-wise (dsproofs are just metadata for
transactions in the mempool, after all).

Test Plan
---

The `bchn-feature-doublespend-proof.py` test was updated to test this new
mechanism.

- `ninja all check && test/functional/test-runner.py bchn-feature-doublespend-proof`
- Also, run the above test with `--nocleanup` and look at the `debug.log`
  for `node0`, to observe the new log messages about saving/loading dsproofs
  from disk.

This undoes some of the changes of the core backport #18710, to minimize
the diff size (keeps more of our code in-tact).

This variable is not really used by our codebase -- we always use the
CCheckQueue, even if no parallel threads are specified (in that case we
use it with the foreground thread).  So this variable had no meaning in
our codebase.

This undoes some of the core backport #17342

This allows one to run the CCheckQueue_RealBlock_32MB* benches with
fine-grained control.

Summary
---

While investigating the potential bottlenecks in validating extremely
large blocks (1 million txns for scalenet), I noticed that the
CCheckQueue parallel signature validator may need further benchmarking
and/or potential optimization.

It is a very complicated piece of machinery that also interacts with the
shared app-wide signature cache under the hood, as such, it could use a
more realistic benchmark.  The existing benchmark set up a very small
data set and didn't actually interact with the signature cache, and did
extremely trivial operations.

This MR is a step towards the goal of better understanding the
bottlenecks  -- it set ups a more realistic benchmark of the CCheckQueue
data structure by using block 556034 which is a 32MB block compiled into
the bench_bitcoin executable.

It then proceeds to actually verify 157783 signatures in parallel, using
all cores and using the real signature checking code and the read signature
cache.

We setup 2 different benchmarks:

- one for the case where we *don't* store the sigcheck results to the
cache -- we simply check sigs in parallel.  This emulates the way IBD
works (IBD doesn't cache result for blocks as it connects them to the tip,
since doing so would just be a waste of time since those sigs will never
be used again once the block is accepted).

- a second benchmark which *does* cache the results of the sigchecks --
this is closer to how "TestBlockValidity" (called when mining) works. It
does the sigchecks but stores the results for later possible use.

Test Plan
---

- `ninja bench_bitcoin`
- `src/bench/bench_bitcoin -filter='CCheckQ.*32MB.*'`

Notice how the two different benchmarks run, and how they differ in
terms of execution speed and also how much total CPU they use. What was
surprising to me after authoring the two benchmarks is that disabling
the storage of the cache results actually *sped up* execution on my
16-core system.  This may be because storing the cached results involves
grabbing a mutex each time and this potentially seriously slows down the
parallel signature checker.  This anomaly does deserve further
investigation as it could be a potential place we can optimize things...

Add VSCode settings to gitignore

See merge request !1219

Summary
---

The -txbroadcastinterval=0 argument should mean
"send txs as fast as possible, without an inv delay".
For values of -txbroadcastinterval=1 or 2, 4, 100, 500, etc
the delay keeps increasing, but for 0 it ends up never sending tx invs!

I am pretty sure this is not the intended design.

This MR fixes the issue (explained in #309).

It also adds a functional test to detect if this issue should ever regress.

Test Plan
---

- On this branch, do: `ninja all && test/functional/test_runner.py bchn-feature-txbroadcastinterval`
  And observe the test runs ok.
- Copy the bchn-feature-txbroadcastinterval.py test to a tmp dir, switch to master,
  copy the test back into ../test/functional/.
  And run the test: `test/functional/test_runner.py bchn-feature-txbroadcastinterval`

You should observe it fail off master but run ok off this MR.

Summary
---

This MR builds on top of previous MRs in the interests of completing the tasks
outlined in #295.

This MR removes the remaining unused logic/code related to the old
ancestor/descendant limits, including unused args as well as unused code in the
wallet.

Summary of Changes
---

- Removal of: -walletrejectlongchains, -limitdescendantcount, -limitancestorcount, -limitdescendantsize, -limitancestorsize
- Removal of wallet coin filter logic related to ancestor/descendant size limits
  (the limits weren't even being calculated or used anymore, so it was dead
  code). This simplified the wallet logic a bit.
- Tests were updated; mainly to remove unused args, but also unit tests were
  updated due to m_ancestors and m_descendants disappearing form the coin filter
  struct(s), etc.
- Removed the static constants related to the defaults for these limits
  (unused).
- Minimum mempool size was previously being calculated based off the descendant
  siz...

Summary
---

This fixes an issue described in #311 where a claimed dsproof orphan
could lead to a situation where the peer that told you about the proof
ends up being erroneously punished with a misbehavior score of +1.

The soluton is to clear the dsproof Entry "nodeId" back to -1 when we
accept a proof and associate it with a real mempool txn.  At this point
tracking the nodeId (which is only used for banning) is no longer needed
and it should be cleared to prevent banning later when the dsproof
completes its lifecycle. Ratonale: if a proof was ever accepeted for any
reason, banning should become impossible in the future.

Note: The normal lifecycle of a dsproof when its txn confirms is to always
become an orphan first (so that it cna be kept around in case of reorg),
before being deleted by the cleaner task after 90 seconds.

Test Plan
---

`ninja all check-all`

Summary
---

Since this upgrade was relay-rules-only, the activation time is no
longer relevant for validating the blockchain itself, thus the time
should no longer be tracked or stored.

Additionally, since the activation logic was removed by a previous MR,
nothing was using this value in the codebase, and it was just dead code.

Summary of changes:

- removed the CLI arg `-tachyonactivationtime`
- removed the function IsTachyonEnabled in activation.cpp since nothing
  other than unit tests was calling it.
- removed the unit test that called this function since it is not needed
- removed `tachyonActivationTime` from the consensus params
- left comments in place about what the activation time once was where the
  old data used to live.

Test Plan
---

`ninja all check-all && ninja bench_bitcoin`

This simplifies the code and removes currently-unused pre-May 15th 2021 code
paths. For the purposes of mempool policy, we pretend that tachyon always
existed and we delete all the unused code paths (and stats) that are no
longer maintained and/or used.

- Removed pre-tachyon support from txmempool and other code

- Undid the delete of the MempoolAncestryTests unit test.
  This unit test, while relying on slow quadratic calculations, is good
  because it does test correctness.
  The slow functions were moved into the mempool_tests.cpp file and are
  just being used for this test, to test correctness.

- Avoid double-copying in ATMP by providing an && overload for addUnchecked
  We provide an rvalue reference overload for CTxMemPool::addUnchecked so
  that we don't have to double-copy the CTxMemPoolEntry coming in from
  AcceptToMemoryPoolWorker

- Removed tachyonLatched from bench/removeforblock.cpp

- Removed limitancestorcount & limitandescendantcount from 2 tests
  These tests were specifying a large ancestor / descendant count so they
  could run.  Since no limit is being enforced anymore, removed these args
  from these tests since they are no longer needed.

- Fixed functional test: mempool_packages.py
  For post-tachyon we simply no longer test the ancestor limit, but invert
  the test such that we test that emitting tx's past the ancestor limit
  works.

- Fixed functional test: mempool_packages_additional.py
  This test was testing the ancestor limits. Now after tachyon they are no
  longer enforced, so the test was updated to instead just exercise the
  code a bit and ensure that the old limit no longer exists.

- Fixed wallet_basic.py test (no longer test chain limits)
  Removed the pre-tachyon testing of wallet & mempool chain limits.

- Fixed functional test: bchn-opreturn.py
  Got rid of activation logic testing -- reduced the test down to simply
  testing for multi-opreturn accept and reject cases.

- Removed pre-tachyon-related unit tests, fixed bench
  All pre-tachyon unit testing that tests ancestor/descendant stats is no
  longer relevant or even tracked by the mempool, so the tests had to go
  (they wouldn't compile anymore anyway).

Test Plan
---------

- `ninja all check-all`

Add more build options

Closes #305

See merge request !1202

[port] Update ZMQ library

See merge request !1209

Summary
---

This test is extremely unreliable in the CI environment due to the CI
env being "noisier" timing/CPU-wise (it likely runs in a VPS).

As such, due to the relatively high false failure rate of this test on
CI, we are better off just disabling it.

A directory was created inside the functional/ test directory called
"disabled".  We can move tests into here to keep the code around in case
we want to fix them, but otherwise disable the tests from ever normally
executing.

Test Plan
---

- Observe CI didn't run this `bchn-rpc-getblocktemplate-timing` test.

Fix "hexstring" param name in "signrawtransactionwithwallet" method

See merge request !1206

Add mempool_fee_delta functional test

See merge request !1207

[doc] Add a clarification about SDK_PATH to depends/README.md

See merge request !1199

Summary
---

This closes issue #300.

Explanation for the failure seen in #300:

It's possible for this functional test to (falsely) fail spuriously due
to the "cached block template" mechanism interfering with the test.  To
ensure that it does not, the fix was to call `self.wait_for_txs` for each
of our two nodes (not just for the first one), which waits until a new
block template is "ready" and until the old one expires (5s max usually).

With this fix the test should no longer spurously fail.

Test Plan
---

- `ninja all && test/functional/test_runner bchn-rpc-getblocktemplatelight`

If you can reliably reproduce the failure,  then by all means try this
commit and try master and see this commit fixes it.

Due to the timing nature of the failure, it's hard to reproduce. I was
unable to do so here on my setup, but I am sure of the underlying issue,
having studied the code again with a critical eye after seeing #300's
error output.

[qa] Remove Win32 cmake build file and target reference in doc

See merge request !1196

Summary
---

This test is very sensitive to timing and needs to be fixed. Until then,
this temporary "fix" is to simply reduce the required success threshold
form 66% to 50%.

It will still fail, but hopefully somewhat less often.

Test Plan
---

`ninja && test/functional/test_runner.py bchn-rpc-getblocktemplate-timing`

Summary
---

This commit closes issue #296. In particular, on ScaleNet, the
statically configured maximum bytes for the disconnect pool is 640MB.
This is bytes of *dynamic* usage.  ScaleNet, with its block size of
256MB serialized size -- may exceed this dynamic memory limit (the
dynamic size multiplier for a tx in the disconnectpool may be ~2.89
or more of its serializd size).  As such, for ScaleNet, it is unsafe to
use a disconnectpool that has a max dynamic size of 640MB (see issue #296
for an explanation as to why).

This commit basically makes the disconnectpool max dynamic size no
longer be a static constant, but calculated at runtime by checking with
the global config object (via GetConfig()).

It ensures the size is max(640MB, MaxMempoolSize).  With default settings,
this change right now only affects scalenet -- for every other network the
old value of 640MB will continue to be used by default. **Unless**, of course,
the  user configures a larger mempool size via `-maxmempool=`.

For scalenet, the new value will be the current maxMempoolSize used on scalenet
(which is 2.5GB by default).

Implementation Details
---

A `static` private member was added to `DisconnectedBlockTransactions`
that calculates the size at runtime, based on current config.

- It just grabs the active config via `GetConfig()`.
  - The alternative would have been to pass down the config object from the
    caller, but I tried that in a branch and the diff was huge.
  - It seemed like overkill as a code change that is only here to fix a minor
    corner case bug discussed in #296.
  - This codebase anyway uses `GetConfig()`, still, all over the place.
- This function returns `uint64_t` and not `size_t` because it is a
  value that relies on the `GetMaxMempoolSize`, which itself is
  `uint64_t`.

Test Plan
---

- `ninja all check-all`

Sadly, there is no test now that tries to reproduce the bug discussed in #296,
but careful study of the code should indicate that the issue is real and
not imagined.

Summary
---

MR !1128 introduced this code but on second reading I realized the
double-copy from this MR can be remove by using `try_emplace` and also
by actually giving the `TxInfo` type a real constructor.

This commit is just a minor code quality commit to reduce the creation
of a temporary.  In most cases the optimizer will elide this temporary
anyway, but for debug builds it may not, and also it's of higher code
quality to just avoid creating it.

Test Plan
---

- `ninja all check-all`

No behavioral or other changes are introduced by this commit. This is
purely a nit/code-quality commit.

Summary
---

This is a small code quality & performance nit.

By using `emplace` instead of `insert`, we avoid the potential creation
of temporary objects (the `std::pair`) on the stack, and instead we constructs
the `std::pair` in-place by passing arguments directly to the final allocated
`std::pair` that lives in the map node.

Test Plan
---

No semantic or behavioral changes are introduced.

- `ninja all check`

Summary
---

As discussed in issue #161, the performance of `removeForBlock` in particular
is a critical path that deserves optimization.

This MR significantly improves the performance of `removeForBlock` from 0% to
as much as 100% or more, depending on the exact mempool layout in question.

The changes introduced also happen to improve the general performance of the
mempool as well.

Minor RPC Change
---

- As a result of these optimizations, the mempool-related RPC calls that
  describe a mempool entry's `spentby` (list of mempool txs that spend a
  particular mempool tx) have a different sort order now. Previously, this list
  would be sorted by txid. Now, it is sorted topologically.
- `getmempooldescendants` and `getmempoolancestors` now also no longer returns a
  list that is sorted by `TxId` but is rather topologically sorted (by
  `entryId`).

Test Plan
---

- `ninja all check-all`

Run these benches on master versus this MR commit:

- `ninja bench_bitcoin && src/bench/bench_bitcoin -filter='(RemoveForBlock.*)|(MempoolAcc.*)|(Reorg.*)|(Generat.*)|(Evict.*)'`

Summary
---

This closes issue #266. The root cause is that the original test author
was misusing threads and/or synchronization primitives. There is no
guarantee that the first main thread runs before the last scheduled
task. As such, sometimes, the last task runs and writes "42" into the
`counter` before the `BOOST_CHECK_EQUAL(counter, 0)` line gets a chance
to be evaluated in the main thread.

The fix is to not rely on undefined behavior here and instead do things
properly.  We just save the counter var to a second atomic and check
everything at the end after the two subordinate tasks have all
definitely finished running.

Test Plan
---

- `ninja all check`
- Try and reproduce the issue in #266 as described. If you can reproduce
  it against master but not here, then you can be happy this is fixed.

An alternative way to test if you *can't* reproduce the failure against master
is to:

1. `git checkout master`
2. Edit `src/test/scheduler_tests.cpp` and insert a
   `std::this_thread::sleep_for(std::chrono::milliseconds(21));` right
*before* the line in the `schedule_every` test that does
`BOOST_CHECK_EQUAL(counter, 0);`. This will reproduce the failure every
time.
3. `ninja test_bitcoin && src/test/test_bitcoin -t scheduler_tests`
4. `git checkout THIS_MR_BRANCH`
5. Add the sleep call in approximately the same place and do steps (2 & 3)
   again.  You should never get a failure now.

Summary
---

This MR is part of on-going work to optimize
`CTxMemPool::removeForBlock`, as discussed in issue #161.

As such, it adds a benchmark for this function versus a full mempool of
~450k unchained txns.

The bench calls removeForBlock repeatedly with block txns from a ~32MB
block versus a very full mempool.

Also an 8MB block version is provided as well.

Test Plan
---

- `ninja all check bench_bitcoin`
- `src/bench/bench_bitcoin -filter='RemoveForBlock.*'`

Summary
---

As described in issue #291, these tests sometimes fail when they shouldn't.
The errors turn out to be timing-related. It's possible for e.g. node0
to send the `firstDSTx` to node1 via the p2p network before we can issue
the `secondDSTx` to node1 ourselves from the Python side via RPC.  When this
happens -- the test would throw a `JSONRPCException` for
`txn-mempool-conflict`.

The fix is to allow for this rare timing-related event and just catch
the exception and proceed.  In either case (no exception or exception),
a dsproof will be generated (which is what we are testing for in this
test).

In order to implement the fix we extended the `TestNode` interface in
the python test framework with a new method, `call_rpc` which takes an
optional kwarg `ignore_error=<str>`. If this kwarg is specified, then
the test will proceed as normal without raising an exception, should
`<std>` match the expected `JSONRPCException` message.

Closes issue #291.

Test Plan
---

- `ninja all check check-functional`

OPTIONAL:

- Try and reproduce the failures described in #291 on master. Then try this
  MR and see it no longer fail.
  - May require a similar setup to #291.
  - A "poor man's hack" for reproducing #291 on a non-slow machine would be
    to add `time.sleep(1)` calls in the bchn-feature-doublespend-proof test
    manually after `firstDSTx` is sent but before `secondDSTx` is sent
    (on master).  Then, try the same with this MR in the same places and see
    it never fail.

Summary
---

As discussed in review and raised in issue #285, the `BatchUpdater`
class hierarchy is no longer needed. It was added originally to
abstract-out the logic for confirmed txs being removed from the mempool,
in order to try out different algorithms.  However, we never ended up making
use of this abstract interface.  During review of the recent mempool changes,
it was decided that this interface probably needs to be reverted back to a
simple direct method inside `CTxMemPool` (for clarity and
maintainability).

As such, this MR does just that.

Additionally, the unused second parameter `nBlockHeight` to
`CTxMemPool::removeForBlock()` was removed, in the interests of code
quality. Should we need this parameter again, we can always bring it
back.  All call-sites were updated to not pass this argument.

This closes #285.

Test Plan
---

- `ninja all check-all`

Summary
---

This commit adds a template class called `Defer` to the `util/` subdirectory.

This class is designed to leverage RAII and execute a lambda at scope
end.

It is used by passing it a lambda (or `std::function`) at construction time
which it will execute when the `Defer` instance is destructed.  This class can
be used as a language idiom to defer the execution of cleanup code at scope end
-- even if an exception is thrown.

Motivation: I noticed in various places in the codebase, very critical
cleanup code is manually executed at scope end -- however if an
exception is thrown by the code, the cleanup code will never run thus
leading to `assert` failures or potential subtle bugs and resource leaks.

What's worse, in some places the critical cleanup code is copy-pased in
several places.

Using this `Defer` idiom one can escape from all of this pain and also guarantee
safety.

This class was already used in this codebase in 2 places (added by me
redundantly in both places, private to each translation unit in which it
was added).

In this commit we just refactor it out to a publicly-visible header in
`util/defer.h` and thus we can re-use this idiom everywhere.

Should this MR be accepted and merged, I do plan on using this idiom in
the future in refactors and code cleanups or in new code.  (I already
know of 1 place where it should be added for safety.)

Test Plan
---

No semantic or other behavioral changes are introduced, however:

- `ninja all check-all`