fa6e6a3f03a38f8b431bf694268ed344d1815b3b doc: Remove confusing assert linter (MarcoFalke)
Pull request description:
The `assert()` documentation and linter are redundant and confusing:
* The source code already refuses to compile with `assert()` disabled.
* They violate the assumptions about `Assert()`, which *requires* side effects.
* The existing linter doesn't enforce the guideline, only checking for `++` and `--` side effects.
Fix all issues by removing the docs and the linter. See also https://github.com/bitcoin/bitcoin/pull/26684#discussion_r1287370102
Going forward everyone is free to use whatever code in this regard they think is the easiest to read. Also, everyone is still free to share style-nits, if they think it is a good use of their time and of the pull request author. Finally, the author is still free to dismiss or ignore this style-nit, or any other style-nit.
ACKs for top commit:
hebasto:
ACK fa6e6a3f03a38f8b431bf694268ed344d1815b3b, I have reviewed the code and it looks OK.
theStack:
ACK fa6e6a3f03a38f8b431bf694268ed344d1815b3b
Tree-SHA512: 686738d71e1316cc95e5d3f71869b55a02bfb137c795cc0875057f4410e564bc8eff03c985a2087b007fb08fc84551c7da1e8b30c7a9c3f2b14e5e44a5970236
edbed31066e3674ba52b8c093ab235625527f383 chainparams: add signet assumeutxo param at height 160_000 (Sjors Provoost)
b8cafe38713cbf10d15459042f7f911bcc1b1e4e chainparams: add testnet assumeutxo param at height 2_500_000 (Sjors Provoost)
99839bbfa7110c7abf22e587ae2f72c9c57d3c85 doc: add note about confusing HaveTxsDownloaded name (James O'Beirne)
7ee46a755f1d57ce9d51975d3b54dc9ac3d08d52 contrib: add script to demo/test assumeutxo (James O'Beirne)
42cae39356fd20d521aaf99aff1ed85856f3c9f3 test: add feature_assumeutxo functional test (James O'Beirne)
0f64bac6030334d798ae205cd7af4bf248feddd9 rpc: add getchainstates (James O'Beirne)
bb0585779472962f40d9cdd9c6532132850d371c refuse to activate a UTXO snapshot if mempool not empty (James O'Beirne)
ce585a9a158476b0ad3296477b922e79f308e795 rpc: add loadtxoutset (James O'Beirne)
62ac519e718eb7a31dca1102a96ba219fbc7f95d validation: do not activate snapshot if behind active chain (James O'Beirne)
9511fb3616b7bbe1d0d2f54a45ea0a650ba0367b validation: assumeutxo: swap m_mempool on snapshot activation (James O'Beirne)
7fcd21544a333ffdf1910b65c573579860be6a36 blockstorage: segment normal/assumedvalid blockfiles (James O'Beirne)
4c3b8ca35c2e4a441264749bb312df2bd054b5b8 validation: populate nChainTx value for assumedvalid chainstates (James O'Beirne)
49ef778158c43859946a592e11ec34fe1b93a5b6 test: adjust chainstate tests to use recognized snapshot base (James O'Beirne)
1019c399825b0d512c1fd751c376d46fed4992b9 validation: pruning for multiple chainstates (James O'Beirne)
373cf91531b84bfdd06fdf8abf4dca228029ce6b validation: indexing changes for assumeutxo (James O'Beirne)
1fffdd76a1bca908f55d73b64983655b14cf7432 net_processing: validationinterface: ignore some events for bg chain (James O'Beirne)
fbe0a7d7ca680358237b6c2369b3fd2b43221113 wallet: validationinterface: only handle active chain notifications (James O'Beirne)
f073917a9e7ba423643dcae0339776470b628f65 validationinterface: only send zmq notifications for active (James O'Beirne)
4d8f4dcb450d31e4847804e62bf91545b949fa14 validation: pass ChainstateRole for validationinterface calls (James O'Beirne)
1e59acdf17309f567c370885f0cf02605e2baa58 validation: only call UpdatedBlockTip for active chainstate (James O'Beirne)
c6af23c5179cc383f8e6c275373af8d11e6a989f validation: add ChainstateRole (James O'Beirne)
9f2318c76cc6986d48e13831cf5bd8dab194fdf4 validation: MaybeRebalanceCaches when chain leaves IBD (James O'Beirne)
434495a8c1496ca23fe35b84499f3daf668d76b8 chainparams: add blockhash to AssumeutxoData (James O'Beirne)
c711ca186f8d8a28810be0beedcb615ddcf93163 assumeutxo: remove snapshot during -reindex{-chainstate} (James O'Beirne)
c93ef43e4fd4fbc1263cdc9e98ae5856830fe89e bugfix: correct is_snapshot_cs in VerifyDB (James O'Beirne)
b73d3bbd23220857bf17cbb6401275bf58013b72 net_processing: Request assumeutxo background chain blocks (Suhas Daftuar)
Pull request description:
- Background and FAQ: https://github.com/jamesob/assumeutxo-docs/tree/2019-04-proposal/proposal
- Prior progress/project: https://github.com/bitcoin/bitcoin/projects/11
- Replaces https://github.com/bitcoin/bitcoin/pull/15606, which was closed due to Github slowness. Original description and commentary can be found there.
---
This changeset finishes the first phase of the assumeutxo project. It makes UTXO snapshots loadable via RPC (`loadtxoutset`) and adds `assumeutxo` parameters to chainparams. It contains all the remaining changes necessary to both use an assumedvalid snapshot chainstate and do a full validation sync in the background.
This may look like a lot to review, but note that
- ~200 lines are a (non-essential) demo shell script
- Many lines are functional test, documentation, and relatively dilute RPC code.
So it shouldn't be as burdensome to review as the linecount might suggest.
- **P2P**: minor changes are made to `init.cpp` and `net_processing.cpp` to make simultaneous IBD across multiple chainstates work.
- **Pruning**: implement correct pruning behavior when using a background chainstate
- **Blockfile separation**: to prevent "fragmentation" in blockfile storage, have background chainstates use separate blockfiles from active snapshot chainstates to avoid interleaving heights and impairing pruning.
- **Indexing**: some `CValidationInterface` events are given with an additional parameter, ChainstateRole, and all indexers ignore events from ChainstateRole::ASSUMEDVALID so that indexation only happens sequentially.
- Have `-reindex` properly wipe snapshot chainstates.
- **RPC**: introduce RPC commands `loadtxoutset` and (hidden) `getchainstates`.
- **Release docs & first assumeutxo commitment**: add notes and a particular assumeutxo hash value for first AU-enabled release.
- This will complete the project and allow use of UTXO snapshots for faster node bootstrap.
The next phase, if it were to be pursued, would be coming up with a way to distribute the UTXO snapshots over the P2P network.
---
### UTXO snapshots
Create your own with `./contrib/devtools/utxo_snapshot.sh`, e.g.
```shell
./contrib/devtools/utxo_snapshot.sh 788000 utxo.dat ./src/bitcoin-cli -datadir=$(pwd)/testdata`)
```
or use the pre-generated ones listed below.
- Testnet: **2'500'000** (Sjors):
- torrent: `magnet:?xt=urn:btih:511e09f4bf853aefab00de5c070b1e031f0ecbe9&dn=utxo-testnet-2500000.dat&tr=udp%3A%2F%2Ftracker.bitcoin.sprovoost.nl%3A6969`
- sha256: `79db4b025448cc0ac388d8589a28eab02de53055d181e34eb47391717aa16388`
- Signet: **160'000** (Sjors):
- torrent: `magnet:?xt=urn:btih:9da986cb27b3980ea7fd06b21e199b148d486880&dn=utxo-signet-160000.dat&tr=udp%3A%2F%2Ftracker.bitcoin.sprovoost.nl%3A6969`
- sha256: `eeeca845385ba91e84ef58c09d38f98f246a24feadaad57fe1e5874f3f92ef8c`
- Mainnet: **800'000** (Sjors):
- Note: this needs the following commit cherry-picked in: 24deb2022b
- torrent: `magnet:?xt=urn:btih:50ee955bef37f5ec3e5b0df4cf0288af3d715a2e&dn=utxo-800000.dat&tr=udp%3A%2F%2Ftracker.bitcoin.sprovoost.nl%3A6969`
### Testing
#### For fun (~5min)
If you want to do a quick test, you can run `./contrib/devtools/test_utxo_snapshots.sh` and follow the instructions. This is mostly obviated by the functional tests, though.
#### For real (longer)
If you'd like to experience a real usage of assumeutxo, you can do that too.
I've cut a new snapshot at height 788'000 (http://img.jameso.be/utxo-788000.dat - but you can do it yourself with `./contrib/devtools/utxo_snapshot.sh` if you want). Download that, and then create a datadir for testing:
```sh
$ cd ~/src/bitcoin # or whatever
# get the snapshot
$ curl http://img.jameso.be/utxo-788000.dat > utxo-788000.dat
# you'll want to do this if you like copy/pasting
$ export AU_DATADIR=/home/${USER}/au-test # or wherever
$ mkdir ${AU_DATADIR}
$ vim ${AU_DATADIR}/bitcoin.conf
dbcache=8000 # or, you know, something high
blockfilterindex=1
coinstatsindex=1
prune=3000
logthreadnames=1
```
Obtain this branch, build it, and then start bitcoind:
```sh
$ git remote add jamesob https://github.com/jamesob/bitcoin
$ git fetch jamesob assumeutxo
$ git checkout jamesob/assumeutxo
$ ./configure $conf_args && make # (whatever you like to do here)
# start 'er up and watch the logs
$ ./src/bitcoind -datadir=${AU_DATADIR}
```
Then, in some other window, load the snapshot
```sh
$ ./src/bitcoin-cli -datadir=${AU_DATADIR} loadtxoutset $(pwd)/utxo-788000.dat
```
You'll see some log messages about headers retrieval and waiting to see the snapshot in the headers chain. Once you get the full headers chain, you'll spend a decent amount of time (~10min) loading the snapshot, checking it, and flushing it to disk. After all that happens, you should be syncing to tip in pretty short order, and you'll see the occasional `[background validation]` log message go by.
In yet another window, you can check out chainstate status with
```sh
$ ./src/bitcoin-cli -datadir=${AU_DATADIR} getchainstates
```
as well as usual favorites like `getblockchaininfo`.
ACKs for top commit:
achow101:
ACK edbed31066e3674ba52b8c093ab235625527f383
Tree-SHA512: 6086fb9a38dc7df85fedc76b30084dd8154617a2a91e89a84fb41326d34ef8e7d7ea593107afba01369093bf8cc91770621d98f0ea42a5b3b99db868d2f14dc2
Introduces ChainstateManager::GetPruneRange().
The prune budget is split evenly between the number of chainstates,
however the prune budget may be exceeded if the resulting shares are
beneath `MIN_DISK_SPACE_FOR_BLOCK_FILES`.
12f7257b8f6ba0de1fb05b598e916c8b14bcab8c doc: Be vague instead of wrong about MALLOC_ARENA_MAX (Tim Ruffing)
Pull request description:
Before this commit, we claim that glibc's malloc implementation uses 2 arenas by default. But that's true only on 32-bit systems, and even there, it uses *up* to 2 arenas.
This commit fixes the wrong statement. The new statement is intentionally vague to reduce our maintenance burden.
For details, see:
https://www.gnu.org/software/libc/manual/html_node/Memory-Allocation-Tunables.html#index-glibc_002emalloc_002earena_005fmax
Noticed in:
https://github.com/bitcoin/bitcoin/pull/27642#issuecomment-1728103427
ACKs for top commit:
fanquake:
ACK 12f7257b8f6ba0de1fb05b598e916c8b14bcab8c
Tree-SHA512: c0ff1e35b682a841e366a1cad26e18ff79a93d97103529be35a972c7dcbb95f5354e7a7b98a86731f491434d64685bb58cc3cc9100f0577d8f75db05e951b09a
a99e9e655a58b2364a74aec5cafb827a73c6b0c4 doc: add release note (ismaelsadeeq)
2b4edf889a4b555c8c7f6793fa5d820e5513ecac test: check `descriptorprocesspsbt` return hex encoded tx (ismaelsadeeq)
c405207a18fdee75a4dea470bb0d13e59e15ce45 rpc: `descriptorprocesspsbt` return hex encoded tx (ismaelsadeeq)
Pull request description:
Coming from [#28414 comment](https://github.com/bitcoin/bitcoin/pull/28414#pullrequestreview-1618684391) Same thing also for `descriptorprocesspsbt`.
Before this PR `descriptorprocesspsbt` returns a boolean `complete` which indicates that the psbt is final, users then have to call `finalizepsbt` to get the hex encoded network transaction.
In this PR if the psbt is complete the return object also has the hex encoded network transaction ready for broadcast with `sendrawtransaction`.
This save users calling `finalizepsbt` with the descriptor, if it is already complete.
ACKs for top commit:
achow101:
ACK a99e9e655a58b2364a74aec5cafb827a73c6b0c4
pinheadmz:
ACK a99e9e655a58b2364a74aec5cafb827a73c6b0c4
ishaanam:
ACK a99e9e655a58b2364a74aec5cafb827a73c6b0c4
Tree-SHA512: c3f1b1391d4df05216c463127cd593f8703840430a99febb54890bc66fadabf9d9530860605f347ec54c1694019173247a0e7a9eb879d3cbb420f9e8d9839b75
While allowing submitted packages to be slightly larger than what
may be allowed in the mempool to allow simpler reasoning
about contextual-less checks vs chain limits.
2e249b922762f19d6ae61edaad062f31bc2849f3 doc: add release note for PR #28414 (Matthew Zipkin)
4614332fc4514f63fcbe9e6de507f7bb9b7e87e9 test: remove unnecessary finalizepsbt rpc calls (ismaelsadeeq)
e3d484b603abff69c6ebfca5cfb78cf82743d090 wallet rpc: return final tx hex from walletprocesspsbt if complete (Matthew Zipkin)
Pull request description:
See https://github.com/bitcoin/bitcoin/pull/28363#discussion_r1315753887
`walletprocesspsbt` currently returns a base64-encoded PSBT and a boolean indicating if the tx is "complete". If it is complete, the base64 PSBT can be finalized with `finalizepsbt` which returns the hex-encoded transaction suitable for `sendrawtransaction`.
With this patch, `walletprocesspsbt` return object will ALSO include the broadcast-able hex string if the tx is already final. This saves users the extra step of calling `finalizepsbt` assuming they have already inspected and approve the transaction from earlier steps.
ACKs for top commit:
ismaelsadeeq:
re ACK 2e249b922762f19d6ae61edaad062f31bc2849f3
BrandonOdiwuor:
re ACK 2e249b9
Randy808:
Tested ACK 2e249b922762f19d6ae61edaad062f31bc2849f3
achow101:
ACK 2e249b922762f19d6ae61edaad062f31bc2849f3
ishaanam:
ACK 2e249b922762f19d6ae61edaad062f31bc2849f3
Tree-SHA512: 229c1103265a9b4248f080935a7ad5607c3be3f9a096a9ab6554093b2cd8aa8b4d1fa55b1b97d3925ba208dbc3ccba4e4d37c40e1491db0d27ba3d9fe98f931e
bd5ae6c66317de39195ddb38cea3ca05bbd99275 doc: misc changes in release-process (fanquake)
d99ba3cc01360f3d251a1d55c73c501822f83c67 doc: filter out merge-script from list of authors (josibake)
472d6f79b9bf7a714c3672ee88e21483a9a46042 doc: remove generate changelog section from release-process.md (fanquake)
5555ecb80ecc1373bc78b3029d1ee33183a9cdc0 doc: remove note to update bips.md version number (fanquake)
Pull request description:
Collection of changes to to simplify / correct the release-process documentation.
I think we could still simplify this further.
For example, we could remove the guix building docs, and defer to `contrib/guix`.
ACKs for top commit:
MarcoFalke:
lgtm ACK bd5ae6c66317de39195ddb38cea3ca05bbd99275
Tree-SHA512: 44bd12dd4f09380daee03fa79f01f9f7e8e2d5cc2fd5ff8c9e85ab54b4ea2b8a35df30ce3bdecfb4ff056cf52822be771ed3419613f68929c122750b1f8c89f9
a733dd79e29068ad1e0532ac42a45188a040a7b9 Remove unused function `reliesOnAssumedValid` (Suhas Daftuar)
d4a11abb1972b54f0babdccfbb2fde97ab885933 Cache block index entry corresponding to assumeutxo snapshot base blockhash (Suhas Daftuar)
3556b850221bc0e597d7dd749d4d47ab58dc8083 Move CheckBlockIndex() from Chainstate to ChainstateManager (Suhas Daftuar)
0ce805b632dcb98944a931f758f76f530f5ce5f2 Documentation improvements for assumeutxo (Ryan Ofsky)
768690b7ce551cd403f8e2a099372915f6022ad4 Fix initialization of setBlockIndexCandidates when working with multiple chainstates (Suhas Daftuar)
d43a1f1a2fa35d377c7a9ad7ab92d1ae325bde3d Tighten requirements for adding elements to setBlockIndexCandidates (Suhas Daftuar)
d0d40ea9a6478d81d7531b7cfc52a8bdaa0883d6 Move block-storage-related logic to ChainstateManager (Suhas Daftuar)
3cfc75366e6596942cbc84f354f42dfd7fc5c073 test: Clear block index flags when testing snapshots (Suhas Daftuar)
272fbc370c4e133d31d9f1d34e327cc265c5fad2 Update CheckBlockIndex invariants for chains based on an assumeutxo snapshot (Suhas Daftuar)
10c05710ce1602d932037f72dc6c4bbc3f6f34ba Add wrapper for adding entries to a chainstate's block index candidates (Suhas Daftuar)
471da5f6e74bac71aeffe2ebc5faff145a6cbcea Move block-arrival information / preciousblock counters to ChainstateManager (Suhas Daftuar)
1cfc887d00c5d1d4281107e3b3ff4641c6c34631 Remove CChain dependency in node/blockstorage (Suhas Daftuar)
fe86a7cd480b32463da900db764d2d11a2bea095 Explicitly track maximum block height stored in undo files (Suhas Daftuar)
Pull request description:
This PR proposes a clean up of the relationship between block storage and the chainstate objects, by moving the decision of whether to store a block on disk to something that is not chainstate-specific. Philosophically, the decision of whether to store a block on disk is related to validation rules that do not require any UTXO state; for anti-DoS reasons we were using some chainstate-specific heuristics, and those have been reworked here to achieve the proposed separation.
This PR also fixes a bug in how a chainstate's `setBlockIndexCandidates` was being initialized; it should always have all the HAVE_DATA block index entries that have more work than the chain tip. During startup, we were not fully populating `setBlockIndexCandidates` in some scenarios involving multiple chainstates.
Further, this PR establishes a concept that whenever we have 2 chainstates, that we always know the snapshotted chain's base block and the base block's hash must be an element of our block index. Given that, we can establish a new invariant that the background validation chainstate only needs to consider blocks leading to that snapshotted block entry as potential candidates for its tip. As a followup I would imagine that when writing net_processing logic to download blocks for the background chainstate, that we would use this concept to only download blocks towards the snapshotted entry as well.
ACKs for top commit:
achow101:
ACK a733dd79e29068ad1e0532ac42a45188a040a7b9
jamesob:
reACK a733dd79e29068ad1e0532ac42a45188a040a7b9 ([`jamesob/ackr/27746.5.sdaftuar.rework_validation_logic`](https://github.com/jamesob/bitcoin/tree/ackr/27746.5.sdaftuar.rework_validation_logic))
Sjors:
Code review ACK a733dd79e29068ad1e0532ac42a45188a040a7b9.
ryanofsky:
Code review ACK a733dd79e29068ad1e0532ac42a45188a040a7b9. Just suggested changes since the last review. There are various small things that could be followed up on, but I think this is ready for merge.
Tree-SHA512: 9ec17746f22b9c27082743ee581b8adceb2bd322fceafa507b428bdcc3ffb8b4c6601fc61cc7bb1161f890c3d38503e8b49474da7b5ab1b1f38bda7aa8668675
6960c81cbfa6208d4098353e53b313e13a21cb49 kernel: Remove Univalue from kernel library (TheCharlatan)
10eb3a9faa977371facacee937b2e6dc26f008e0 kernel: Split ParseSighashString (TheCharlatan)
Pull request description:
Besides the build system changes, this is a mostly move-only change for moving the few UniValue-related functions out of kernel files.
UniValue is not required by any of the kernel components and a JSON library should not need to be part of a consensus library.
ACKs for top commit:
achow101:
ACK 6960c81cbfa6208d4098353e53b313e13a21cb49
theuni:
Re-ACK 6960c81cbfa6208d4098353e53b313e13a21cb49
stickies-v:
re-ACK 6960c81cbf
Tree-SHA512: d92e4cb4e12134c94b517751bd746d39f9b8da528ec3a1c94aaedcce93274a3bae9277832e8a7c0243c13df0397ca70ae7bbb24ede200018c569f8d81103c1da
Nobody is pushing direct to guix.sigs, nor should they, as that
bypasses CI.
Use a newer example for the testing issue.
Don't duplicate the bitcoincore.org doc instructions.
The thread does not only load blocks, it loads the mempool and,
in a future commit, will start the indexes as well.
Also, renamed the 'ThreadImport' function to 'ImportBlocks'
And the 'm_load_block' class member to 'm_thread_load'.
-BEGIN VERIFY SCRIPT-
sed -i "s/ThreadImport/ImportBlocks/g" $(git grep -l ThreadImport -- ':!/doc/')
sed -i "s/loadblk/initload/g" $(git grep -l loadblk -- ':!/doc/release-notes/')
sed -i "s/m_load_block/m_thread_load/g" $(git grep -l m_load_block)
-END VERIFY SCRIPT-
11900e5a8aa4647e50e8d97fc9a8b35e9e20772b doc: simplify the router options in doc/i2p.md (Jon Atack)
b505d59326ce04645ebef06b21286a4ffbcd3a23 doc: clarify when and how to launch the SAM bridge in doc/i2p.md (Jon Atack)
Pull request description:
1. Clarify when and how to launch the SAM application bridge to address user questions and https://github.com/bitcoin/bitcoin/issues/22759#issuecomment-1599449753. The bridge is not enabled by default in the Java I2P Router, and the relevant info is somewhat difficult to find in its documentation.
2. Remove a duplicate sentence and link (the preceding paragraph begins with the same sentence and link).
3. Simplify the router options:
- the Java I2P router and i2pd are the two routers have been heavily tested with Bitcoin Core and are what node operators and node software packages are using
- [i2p-zero](https://github.com/i2p-zero/i2p-zero) hasn't been updated since July 2021 and its last release was in December 2020
- the other routers in the wikipedia page are niche and I haven't heard anyone report using them
ACKs for top commit:
fanquake:
ACK 11900e5a8aa4647e50e8d97fc9a8b35e9e20772b
Tree-SHA512: add5f40004feddc7829d658a5de30422f144ab8fa09ff1396d5d263ee86e39803595a2c50e2aa601d0df6da5eff4358c3f7f17e039dcd4952cb306596c6c0397
3c83b1d884b419adece95b335b6e956e7459a7ef doc: Add release note for wallet loading changes (Andrew Chow)
2636844f5353797a0b8e40a879652a0d345172ad walletdb: Remove loading code where the database is iterated (Andrew Chow)
cd211b3b9965b5070d68adc1a03043d82d904d5b walletdb: refactor decryption key loading (Andrew Chow)
31c033e5ca3b65f4f5345d5aa17aafedd637ef4f walletdb: refactor defaultkey and wkey loading (Andrew Chow)
c978c6d39cdeb78fc4720767b943d03d6a9a36d8 walletdb: refactor active spkm loading (Andrew Chow)
6fabb7fc99e60584d5f3a2cb01d39f761769a25d walletdb: refactor tx loading (Andrew Chow)
abcc13dd24889bc1c6af7b10da1da96d86aeafed walletdb: refactor address book loading (Andrew Chow)
405b4d914712b5de3b230a0e2960e89f6a0a2b2a walletdb: Refactor descriptor wallet records loading (Andrew Chow)
30ab11c49793d5d55d66c4dedfa576ae8fd6129c walletdb: Refactor legacy wallet record loading into its own function (Andrew Chow)
9e077d9b422ac3c371fe0f63da40e5092171a25e salvage: Remove use of ReadKeyValue in salvage (Andrew Chow)
ad779e9ece9829677c1735d8865f14b23459da80 walletdb: Refactor hd chain loading to its own function (Andrew Chow)
72c2a54ebb99fa3d91d7d15bd8a38a8d16e0ea6c walletdb: Refactor encryption key loading to its own function (Andrew Chow)
3ccde4599b5150577400c4fa9029f4146617f751 walletdb: Refactor crypted key loading to its own function (Andrew Chow)
7be10adff36c0dc49ae56ac571bb033cba7a565b walletdb: Refactor key reading and loading to its own function (Andrew Chow)
52932c5adb29bb9ec5f0bcde9a31b74113a20651 walletdb: Refactor wallet flags loading (Andrew Chow)
01b35b55a119dc7ac915fc621ecebcd5c50ccb55 walletdb: Refactor minversion loading (Andrew Chow)
Pull request description:
Currently when we load a wallet, we just iterate through all of the records in the database and add them completely statelessly. However we have some records which do rely on other records being loaded before they are. To deal with this, we use `CWalletScanState` to hold things temporarily until all of the records have been read and then we load the stateful things.
However this can be slow, and with some future improvements, can cause some pretty drastic slowdowns to retain this pattern. So this PR changes the way we load records by choosing to load the records in a particular order. This lets us do things such as loading a descriptor record, then finding and loading that descriptor's cache and key records. In the future, this will also let us use `IsMine` when loading transactions as then `IsMine` will actually be working as we now always load keys and descriptors before transactions.
In order to get records of a specific type, this PR includes some refactors to how we do database cursors. Functionality is also added to retrieve a cursor that will give us records beginning with a specified prefix.
Lastly, one thing that iterating the entire database let us do was to find unknown records. However even if unknown records were found, we would not do anything with this information except output a number in a log line. With this PR, we would no longer be aware of any unknown records. This does not change functionality as we don't do anything with unknown records, and having unknown records is not an error. Now we would just not be aware that unknown records even exist.
ACKs for top commit:
MarcoFalke:
re-ACK 3c83b1d884b419adece95b335b6e956e7459a7ef 🍤
furszy:
reACK 3c83b1d8
ryanofsky:
Code review ACK 3c83b1d884b419adece95b335b6e956e7459a7ef. Just Marco's suggested error handling fixes since last review
Tree-SHA512: 15fa56332fb2ce4371db468a0c674ee7a3a8889c8cee9f428d06a7d1385d17a9bf54bcb0ba885c87736841fe6a5c934594bcf4476a473616510ee47862ef30b4
- the Java I2P router and i2pd are the two routers have been heavily tested
with Bitcoin Core and are what people and node software packages use
- i2p-zero (https://github.com/i2p-zero/i2p-zero) hasn't been updated since
July 2021 and its last release was in December 2020
- the other routers in the wikipedia page are niche
The SAM application bridge is not enabled by default in the Java I2P Router,
and the relevant info is somewhat difficult to find in its documentation.
Also, remove a duplicate sentence; the preceding paragraph begins with the same.
daa5a658c0e79172e4dea0758246f11281790d29 refactor: rename BCLog::BLOCKSTORE to BLOCKSTORAGE (Jon Atack)
cf622b214bfe0a97e403f1e9dc54bf5bbfc59fc3 doc: release note re raising on invalid -debug/debugexclude/loglevel (Jon Atack)
6cb1c66041ee14dbedad3aeeb90190ea5dddf917 init: remove config option names from translated -loglevel strings (Jon Atack)
25478292726dd7208b22a8924c8f1fdeac5c33f5 test: -loglevel raises on invalid values (Jon Atack)
a9c295888b82c86ef4629aa2d9061ea152b48f20 init: raise on invalid loglevel config option (Jon Atack)
b0c3995393c592fa96306e077ed64e65d5400882 test: -debug and -debugexclude raise on invalid values (Jon Atack)
4c3c19d943a0a4cf191495f6ebe9b964835607a4 init: raise on invalid debug/debugexclude config options (Jon Atack)
Pull request description:
and rename BCLog::BLOCKSTORE to BLOCKSTORAGE so the enum is the same as its value like the other BCLog enums.
Per discussion in bitcoin-core-dev IRC today from https://bitcoin-irc.chaincode.com/bitcoin-core-dev/2023-05-11#921458.
ACKs for top commit:
achow101:
ACK daa5a658c0e79172e4dea0758246f11281790d29
ryanofsky:
Code review ACK daa5a658c0e79172e4dea0758246f11281790d29. Just translated string template cleanup since last review
pinheadmz:
re-ACK daa5a658c0e79172e4dea0758246f11281790d29
Tree-SHA512: 4c107a93d8e8ce4e2ee81d44aec672526ca354ec390b241221067f68204beac8b4ba7a65748bcfa124ff2245c4307fa9243ec4fe0b464d0fa69c787fb322c3cc
5524fa00faebfe040f126a4152640f9e9ed572b1 doc: add release note about removal of `deprecatedrpc=walletwarningfield` flag (Sebastian Falbesoner)
5c77db73542fe4c76fd53526ae560d56dde5f830 Restorewallet/createwallet help documentation fixups/improvements (Jon Atack)
a00ae31fccba63d5fd409ffb39c1622df2ea3e8c rpc: remove deprecated "warning" field from {create,load,restore,unload}wallet (Sebastian Falbesoner)
Pull request description:
The "warning" string field for wallet creating/loading RPCs (`createwallet`, `loadwallet`, `unloadwallet` and `restorewallet`) has been deprecated with the configuration option `-deprecatedrpc=walletwarningfield` in PR #27279 (released in v25.0). For the next release v26.0, the field and the configuration option can be removed.
ACKs for top commit:
achow101:
ACK 5524fa00faebfe040f126a4152640f9e9ed572b1
jonatack:
ACK 5524fa00faebfe040f126a4152640f9e9ed572b1
Tree-SHA512: 8212f72067d08095304018b8a95d2ebef630004b65123483fbbfb078cc5709c2d825bbc35b16ea5f6b28ae7377347382d7e9afaf7bdbf0575d2c229d970784de
67b7fecacd0489809690982c89ba2d0acdca938c [mempool] clear mapDeltas entry if prioritisetransaction sets delta to 0 (glozow)
c1061acb9d502cdf8c6996c818d9a8a281cbe40c [functional test] prioritisation is not removed during replacement and expiry (glozow)
0e5874f0b06114d9b077e0ff582915e4f83059e6 [functional test] getprioritisedtransactions RPC (glozow)
99f8046829f699ff2eace266aa8cea1d9f7cb65a [rpc] add getprioritisedtransactions (glozow)
9e9ca36c80013749faaf2aa777d52bd07d9d24ec [mempool] add GetPrioritisedTransactions (glozow)
Pull request description:
Add an RPC to get prioritised transactions (also tells you whether the tx is in mempool or not), helping users clean up `mapDeltas` manually. When `CTxMemPool::PrioritiseTransaction` sets a delta to 0, remove the entry from `mapDeltas`.
Motivation / Background
- `mapDeltas` entries are never removed from mapDeltas except when the tx is mined in a block or conflicted.
- Mostly it is a feature to allow `prioritisetransaction` for a tx that isn't in the mempool {yet, anymore}. A user can may resbumit a tx and it retains its priority, or mark a tx as "definitely accept" before it is seen.
- Since #8448, `mapDeltas` is persisted to mempool.dat and loaded on restart. This is also good, otherwise we lose prioritisation on restart.
- Note the removal due to block/conflict is only done when `removeForBlock` is called, i.e. when the block is received. If you load a mempool.dat containing `mapDeltas` with transactions that were mined already (e.g. the file was saved prior to the last few blocks), you don't delete them.
- Related: #4818 and #6464.
- There is no way to query the node for not-in-mempool `mapDeltas`. If you add a priority and forget what the value was, the only way to get that information is to inspect mempool.dat.
- Calling `prioritisetransaction` with an inverse value does not remove it from `mapDeltas`, it just sets the value to 0. It disappears on a restart (`LoadMempool` checks if delta is 0), but that might not happen for a while.
Added together, if a user calls `prioritisetransaction` very regularly and not all those transactions get mined/conflicted, `mapDeltas` might keep lots of entries of delta=0 around. A user should clean up the not-in-mempool prioritisations, but that's currently difficult without keeping track of what those txids/amounts are.
ACKs for top commit:
achow101:
ACK 67b7fecacd0489809690982c89ba2d0acdca938c
theStack:
Code-review ACK 67b7fecacd0489809690982c89ba2d0acdca938c
instagibbs:
code review ACK 67b7fecacd0489809690982c89ba2d0acdca938c
ajtowns:
ACK 67b7fecacd0489809690982c89ba2d0acdca938c code review only, some nits
Tree-SHA512: 9df48b622ef27f33db1a2748f682bb3f16abe8172fcb7ac3c1a3e1654121ffb9b31aeaad5570c4162261f7e2ff5b5912ddc61a1b8beac0e9f346a86f5952260a
