Work decoupled from #28574.
Instead of performing multiple single write operations per spkm setup call, this PR batches them all within a single atomic db txn.
Speeding up the process and preventing the wallet from entering an inconsistent state if any of the intermediate transactions fail (which shouldn’t happen but.. if it does, it is better to not store any spkm rather than storing them partially).
To compare the changes, added benchmark in the first commit.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For detailed information about the code coverage, see the test coverage report.
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | Sjors, achow101, BrandonOdiwuor, theStack |
| Approach ACK | S3RK |
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
Reviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.
Concept ACK
Didn’t look deeper at the code yet, but ran the new benchmark (on an SSD) for comparison via
0$ ./src/bench/bench_bitcoin -filter=WalletCreatePlain\|WalletCreateEncrypted
master (commit bb4554c81e0d819d74996f89cbb9c00476aedf8c which adds the benchmark)
| ns/op | op/s | err% | total | benchmark |
|---|---|---|---|---|
| 797,011,189.00 | 1.25 | 0.6% | 8.74 | WalletCreateEncrypted |
| 391,984,469.00 | 2.55 | 1.1% | 4.30 | WalletCreatePlain |
PR head (1901360ca52c6563a40dc78633cc492a822ac7a9):
| ns/op | op/s | err% | total | benchmark |
|---|---|---|---|---|
| 724,523,571.00 | 1.38 | 0.5% | 7.97 | WalletCreateEncrypted |
| 319,389,684.00 | 3.13 | 0.8% | 3.50 | WalletCreatePlain |
Looks like a rough ~10% speedup for encrypted and ~22% speedup for plain wallets on my machine. Even without speedups, I think it’s good practice to avoid entering an inconsistent state, as mentioned in the PR description.
581@@ -582,6 +582,9 @@ class DescriptorScriptPubKeyMan : public ScriptPubKeyMan
582 // Fetch the SigningProvider for a given index and optionally include private keys. Called by the above functions.
583 std::unique_ptr<FlatSigningProvider> GetSigningProvider(int32_t index, bool include_private = false) const EXCLUSIVE_LOCKS_REQUIRED(cs_desc_man);
584
585+ //! Same as 'TopUp' but designed for use within a batch transaction context
586+ bool TopUpWithDb(WalletBatch& batch, unsigned int size = 0);
TopUpWithDB
3558- AddActiveScriptPubKeyMan(id, t, internal);
3559+ AddActiveScriptPubKeyManWithDb(batch, id, t, internal);
3560 }
3561 }
3562+
3563+ // Ensure information is dumped to disk
ACK 1901360ca52c6563a40dc78633cc492a822ac7a9
Not sure if b85b75d6470841813c5844b2e9c883360f1ad94a is needed since we’re getting rid of the legacy wallet anyway. But it looks correct.
Ran bench on MacBook Pro 2019 (2,3 GHz 8-Core Intel Core i9, SSD):
Before:
0src/bench/bench_bitcoin -filter="WalletCreate[E|P].*" -min-time=30000
| ns/op | op/s | err% | total | benchmark |
|---|---|---|---|---|
| 924,961,364.67 | 1.08 | 1.8% | 30.41 | WalletCreateEncrypted |
| 537,964,679.50 | 1.86 | 2.3% | 35.03 | WalletCreatePlain |
After:
| ns/op | op/s | err% | total | benchmark |
|---|---|---|---|---|
| 777,090,017.50 | 1.29 | 1.3% | 32.65 | WalletCreateEncrypted |
| 361,932,639.25 | 2.76 | 0.6% | 33.34 | WalletCreatePlain |
Here’s a commit that batches the descriptor import for external signers: https://github.com/Sjors/bitcoin/commit/286883a68d24e3600d9e608ccdea55763c50dde1 (I had to make TopupWithDB() public)
Instead of performing multiple atomic write
operations per descriptor setup call, batch
them all within a single atomic db txn.
Instead of performing multiple atomic write
operations per legacy spkm setup call, batch
them all within a single atomic db txn.
Instead of doing one db transaction per descriptor setup,
batch all descriptors' setup writes in a single db txn.
Speeding up the process and preventing the wallet from entering
an inconsistent state if any of the intermediate transactions
fail.
Co-authored-by: furszy <matiasfurszyfer@protonmail.com>
Updated per feedback, thanks for the review @Sjors.
Here’s a commit that batches the descriptor import for external signers: https://github.com/Sjors/bitcoin/commit/286883a68d24e3600d9e608ccdea55763c50dde1 (I had to make TopupWithDB() public)
Thanks. Pulled the commit and made two changes to it:
TopupWithDB() public, made it protected. The ExternalSignerScriptPubKeyMan class is derived from DescriptorScriptPubKeyMan.SetupDescriptor() function from DescriptorScriptPubKeyMan. Only ExternalSignerScriptPubKeyMan utilizes it.
re-utACK f05302427386fe63f4929a7198652cb1e4ab3bcc
CI failure seems spurious
30+
31+ DatabaseStatus status;
32+ bilingual_str error_string;
33+ std::vector<bilingual_str> warnings;
34+
35+ fs::path wallet_path = test_setup->m_path_root / strprintf("test_wallet_%d", random.rand32()).c_str();
m_path_root should already be (more) random. If not, it would be good to remove. Also, to reduce the use of c_str, which (in other parts of the code) is fragile and can lead to bugs. So git grep c_str will be less verbose if unused used of c_str are removed.
no, it doesn’t. Initially, the wallet_path was inside the bench.run() lambda to create wallets under different paths. This was to avoid flushing the wallet to disk and removing the directory within the benchmark execution (which has nothing to do with what we are trying to benchmark). Then, at the end, I changed the approach to occupy less memory.
But.. thinking it further now, we could go back to the previous approach and not be concerned about memory by setting the max iterations number. Also, probably for another working path, wouldn’t be bad to introduce a way to clean up stuff within the benchmark execution that is deliberately not included in the bench results.
Thoughts?
Also, probably for another working path, wouldn’t be bad to introduce a way to clean up stuff within the benchmark execution that is deliberately not included in the bench results.
Sgtm, but if the cleanup only takes a very short time, I think it is fine to just leave as-is.