dd9633b516 test: wallet, add coverage for watch-only raw sh script migration (furszy)
cc781a2180 descriptor: InferScript, do not return top-level only func as sub descriptor (furszy)
286e0c7d5e wallet: loading, log descriptor parsing error details (furszy)
Pull request description:
Linked to #28057.
Currently, the `InferScript` function returns an invalid descriptor when it tries to infer a p2sh-p2pkh script whose pubkey is not known by the wallet.
This behavior occurs because the inference process bypasses the `pkh` subscript when the pubkey is not contained by the wallet (no pubkey provider), interpreting it as a `sh(addr(ADDR))` descriptor. Then, the failure arises because the `addr()` function is restricted to being used only at the top level.
For reviewers, would recommend to start by examining the functional test to understand the context and the circumstances on which this can result in a fatal error (e.g. during the migration process).
ACKs for top commit:
achow101:
ACK dd9633b516
darosior:
utACK dd9633b516
Tree-SHA512: 61e763206c604c372019d2c36e31684f3dddf81f8b154eb9aba5cd66d8d61bda457ed4e591613eb6ce6c76cf7c3f11764abc6cd727a7c2b6414f1065783be032
e.g. sh(addr(ADDR)) or sh(raw(HEX)) are invalid descriptors.
Making sh and wsh top level functions to return addr/raw descriptors when
the subscript inference fails.
fa6245da60 fuzz: Generate process_message targets individually (MarcoFalke)
fa1471e575 refactor: Remove duplicate allNetMessageTypesVec (MarcoFalke)
Pull request description:
Now that `LIMIT_TO_MESSAGE_TYPE` is a runtime setting after commit 927b001502, it shouldn't hurt to also generate each message type individually. Something similar was done for the `rpc` target in commit cf4da5ec29.
ACKs for top commit:
stickies-v:
re-crACK fa6245da60
brunoerg:
reACK fa6245da60
Tree-SHA512: 8f3ec71bab89781f10820a0e027fcde8949f3333eb19a30315aaad6f90f5167028113cea255b2d60b700da817c7eaac20b7b4c92f931052d7f5c2f148d33aa5a
e8c31f135c tests: Test for bumping single output transaction (Andrew Chow)
4f4d4407e3 test: Test bumpfee reduce_output (Andrew Chow)
7d83502d3d bumpfee: Allow original change position to be specified (Andrew Chow)
Pull request description:
When bumping the transaction fee, we will try to find the change output of the transaction in order to have an output whose value we can modify so that we can meet the target feerate. However we do not always find the change output which can cause us to unnecessarily add an additional output to the transaction. We can avoid this issue by outsourcing the determination of change to the user if they so desire.
This PR adds a `orig_change_pos` option to bumpfee which the user can use to specify the index of the change output.
Fixes#11233Fixes#20795
ACKs for top commit:
ismaelsadeeq:
re ACK e8c31f135c
pinheadmz:
re-ACK e8c31f135c
furszy:
Code review ACK e8c31f13
Tree-SHA512: 3a230655934af17f7c1a5953fafb5ef0d687c21355cf284d5e98fece411f589cd69ea505f06d6bdcf82836b08d268c366ad2dd30ae3d71541c9cdf94d1f698ee
f1d807e383 Add more tests for the BIP21 implementation (Kiminuo)
Pull request description:
This PR is an attempt to make it clear how the current BIP21 implementation behaves in Bitcoin Core. Especially, I'm interested whether one can specify multiple `amount` (`message`, etc.) parameters.
My primary end goal is to answer [this question of mine](https://bitcoin.stackexchange.com/questions/118654/how-to-interpret-bip21-uri-with-amount-specified-twice/) but I figured that maybe it's worth a PR. If not, I'll close the PR.
ACKs for top commit:
MarcoFalke:
lgtm ACK f1d807e383
kevkevinpal:
ACK [f1d807e](f1d807e383)
Tree-SHA512: d287809d47c5cfc667f850927bfd969bd345a996d3d53a4c26ef0ffd29eb75ef53358692a15f9a0493ec9e1c101123b6584572e25f87bcb98ff67f6b6c166de4
7d92b1430a refactor: use Span for SipHash::Write (Sebastian Falbesoner)
Pull request description:
This simple refactoring PR changes the interface for the `SipHash` arbitrary-data `Write` method to take a `Span<unsigned char>` instead of having to pass data and length. (`Span<std::byte>` seems to be more modern, but vectors of `unsigned char` are still used prety much everywhere where SipHash is called, and I didn't find it very appealing having to clutter the code with `Make(Writable)ByteSpan` helpers).
ACKs for top commit:
sipa:
utACK 7d92b1430a
MarcoFalke:
lgtm ACK 7d92b1430a
achow101:
ACK 7d92b1430a
Tree-SHA512: f17a27013c942aead4b09f5a64e0c3ff8dbc7e83fe63eb9a2e3ace8be9921c9cbba3ec67e3e83fbe3332ca941c42370efd059e702c060f9b508307e9657c66f2
f6a26196cf Added `longpollid` and `data` params to `template_request` #27998 (Rhythm Garg)
Pull request description:
This PR will add the optional parameters `longpollid` and `data` to `template_request` as they were missing when calling `help getblocktemplate` in RPCHelpMan.
I request the maintainers to review this and let me know about any mistakes in the descriptions of the parameters.
This PR refers to the issue #27998
ACKs for top commit:
ItIsOHM:
> tACK [f6a2619](f6a26196cf)
russeree:
tACK f6a26196cf
stickies-v:
tACK f6a26196cf
Tree-SHA512: 6c592db59cb11b2d031ce5265c547fa296266278f6c25f96afe18a420e0d547f4d483e0f66de75d52c0c319ac1585f3558b9f70c12ef208c96ec96a51f786c6a
5080c9c25f build: adapt Windows builds for libsecp256k1 build changes (fanquake)
ff061fde18 Squashed 'src/secp256k1/' changes from 705ce7ed8c..c545fdc374 (fanquake)
Pull request description:
Includes https://github.com/bitcoin-core/secp256k1/pull/1378. Which fixes#28079.
Adapts Windows build for https://github.com/bitcoin-core/secp256k1/pull/1367.
ACKs for top commit:
hebasto:
ACK 5080c9c25f, I've made the `src/secp256k1` subtree update locally and got zero diff with this PR branch.
jonasnick:
ACK 5080c9c25f
Tree-SHA512: 37915d420ebacefc6bc82c2511bff3d6884e01d5c92795f19cd61862f96b30aa1fe768aeabec128c9d25c1d8bc62b46b46969626067266074b39566ad9e2f5ba
1cd45d4e08 test: move random.h include header from setup_common.h to cpp (Jon Atack)
1b246fdd14 test: move remaining random test util code from setup_common to random (jonatack)
Pull request description:
and drop the `util/random` dependency on `util/setup_common`. This improves code separation and allows `util/setup_common` to call `util/random` functions without creating a circular dependency, thereby addressing https://github.com/bitcoin/bitcoin/pull/26940#issuecomment-1497266140 by glozow (thanks!)
ACKs for top commit:
MarcoFalke:
lgtm ACK 1cd45d4e08🌂
Tree-SHA512: 6ce63d9103ba9b04eebbd8ad02fe9aa79e356296533404034a1ae88e9b7ca0bc9a5c51fd754b71cf4e7b55b18bcd4d5474b2d588edee3851e3b3ce0e4d309a93
c545fdc374 Merge bitcoin-core/secp256k1#1298: Remove randomness tests
b40e2d30b7 Merge bitcoin-core/secp256k1#1378: ellswift: fix probabilistic test failure when swapping sides
c424e2fb43 ellswift: fix probabilistic test failure when swapping sides
907a67212e Merge bitcoin-core/secp256k1#1313: ci: Test on development snapshots of GCC and Clang
0f7657d59c Merge bitcoin-core/secp256k1#1366: field: Use `restrict` consistently in fe_sqrt
cc55757552 Merge bitcoin-core/secp256k1#1340: clean up in-comment Sage code (refer to secp256k1_params.sage, update to Python3)
600c5adcd5 clean up in-comment Sage code (refer to secp256k1_params.sage, update to Python3)
981e5be38c ci: Fix typo in comment
e9e9648219 ci: Reduce number of macOS tasks from 28 to 8
609093b387 ci: Add x86_64 Linux tasks for gcc and clang snapshots
1deecaaf3b ci: Install development snapshots of gcc and clang
b79ba8aa4c field: Use `restrict` consistently in fe_sqrt
c9ebca95f9 Merge bitcoin-core/secp256k1#1363: doc: minor ellswift.md updates
afd7eb4a55 Merge bitcoin-core/secp256k1#1371: Add exhaustive tests for ellswift (with create+decode roundtrip)
2792119278 Add exhaustive test for ellswift (create+decode roundtrip)
c7d900ffd1 doc: minor ellswift.md updates
332af315fc Merge bitcoin-core/secp256k1#1344: group: save normalize_weak calls in `secp256k1_ge_is_valid_var`/`secp256k1_gej_eq_x_var`
9e6d1b0e9b Merge bitcoin-core/secp256k1#1367: build: Improvements to symbol visibility logic on Windows (attempt 3)
0aacf64352 Merge bitcoin-core/secp256k1#1370: Corrected some typos
b6b9834e8d small fixes
07c0e8b82e group: remove unneeded normalize_weak in `secp256k1_gej_eq_x_var`
3fc1de5c55 Merge bitcoin-core/secp256k1#1364: Avoid `-Wmaybe-uninitialized` when compiling with `gcc -O1`
fb758fe8d6 Merge bitcoin-core/secp256k1#1323: tweak_add: fix API doc for tweak=0
c6cd2b15a0 ci: Add task for static library on Windows + CMake
020bf69a44 build: Add extensive docs on visibility issues
0196e8ade1 build: Introduce `SECP256k1_DLL_EXPORT` macro
9f1b1904a3 refactor: Replace `SECP256K1_API_VAR` with `SECP256K1_API`
ae9db95cea build: Introduce `SECP256K1_STATIC` macro for Windows users
7966aee31d Merge bitcoin-core/secp256k1#1369: ci: Print commit in Windows container
a7bec34231 ci: Print commit in Windows container
249c81eaa3 Merge bitcoin-core/secp256k1#1368: ci: Drop manual checkout of merge commit
98579e297b ci: Drop manual checkout of merge commit
5b9f37f136 ci: Add `CFLAGS: -O1` to task matrix
a6ca76cdf2 Avoid `-Wmaybe-uninitialized` when compiling with `gcc -O1`
0fa84f869d Merge bitcoin-core/secp256k1#1358: tests: introduce helper for non-zero `random_fe_test()` results
5a95a268b9 tests: introduce helper for non-zero `random_fe_test` results
304421d57b tests: refactor: remove duplicate function `random_field_element_test`
3aef6ab8e1 Merge bitcoin-core/secp256k1#1345: field: Static-assert that int args affecting magnitude are constant
4494a369b6 Merge bitcoin-core/secp256k1#1357: tests: refactor: take use of `secp256k1_ge_x_on_curve_var`
799f4eec27 Merge bitcoin-core/secp256k1#1356: ci: Adjust Docker image to Debian 12 "bookworm"
c862a9fb49 ci: Adjust Docker image to Debian 12 "bookworm"
a1782098a9 ci: Force DWARF v4 for Clang when Valgrind tests are expected
7d8d5c86df tests: refactor: take use of `secp256k1_ge_x_on_curve_var`
8a7273465b Help the compiler prove that a loop is entered
fd491ea1bb Merge bitcoin-core/secp256k1#1355: Fix a typo in the error message
ac43613d25 Merge bitcoin-core/secp256k1#1354: Add ellswift to CHANGELOG
67887ae65c Fix a typo in the error message
926dd3e962 Merge bitcoin-core/secp256k1#1295: abi: Use dllexport for mingw builds
10836832e7 Merge bitcoin-core/secp256k1#1336: Use `__shiftright128` intrinsic in `secp256k1_u128_rshift` on MSVC
7c7467ab7f Refer to ellswift.md in API docs
c32ffd8d8c Add ellswift to CHANGELOG
3c1a0fd37f Merge bitcoin-core/secp256k1#1347: field: Document return value of fe_sqrt()
5779137457 field: Document return value of fe_sqrt()
be8ff3a02a field: Static-assert that int args affecting magnitude are constant
efa76c4bf7 group: remove unneeded normalize_weak in `secp256k1_ge_is_valid_var`
5b7bf2e9d4 Use `__shiftright128` intrinsic in `secp256k1_u128_rshift` on MSVC
05873bb6b1 tweak_add: fix API doc for tweak=0
6ec3731e8c Simplify test PRNG implementation
fb5bfa4eed Add static test vector for Xoshiro256++
723e8ca8f7 Remove randomness tests
bc7c8db179 abi: Use dllexport for mingw builds
git-subtree-dir: src/secp256k1
git-subtree-split: c545fdc374964424683d9dac31a828adedabe860
c7db88af71 descriptor: assert we never parse a sane miniscript with no pubkey (Antoine Poinsot)
a49402a9ec qa: make sure we don't let unspendable Miniscript descriptors be imported (Antoine Poinsot)
639e3b6c97 descriptor: refuse to parse unspendable miniscript descriptors (Antoine Poinsot)
e3280eae1b miniscript: make GetStackSize() and GetOps() return optionals (Antoine Poinsot)
Pull request description:
`IsSane()` in Miniscript does not ensure a Script is actually spendable. This is an issue as we would accept any sane Miniscript when parsing a descriptor. Fix this by explicitly checking a Miniscript descriptor is both sane and spendable when parsing it.
This bug was exposed due to a check added in #22838 (https://github.com/bitcoin/bitcoin/pull/22838#discussion_r1226859880) that triggered a fuzz crash (https://github.com/bitcoin/bitcoin/pull/22838#issuecomment-1612510057).
ACKs for top commit:
sipa:
utACK c7db88af71
achow101:
ACK c7db88af71
Tree-SHA512: e79bc9f7842e98a4e8f358f05811fca51b15b4b80a171c0d2b17cf4bb1f578a18e4397bc2ece9817d392e0de0196ee6a054b7318441fd3566dd22e1f03eb64a5
4e5c933f6a Switch all callers from poly1305_auth to Poly1305 class (Pieter Wuille)
8871f7d1ae tests: add more Poly1305 test vectors (Pieter Wuille)
40e6c5b9fc crypto: add Poly1305 class with std::byte Span interface (Pieter Wuille)
50269b391f crypto: switch poly1305 to incremental implementation (Pieter Wuille)
Pull request description:
Our current Poly1305 code (src/crypto/poly1305.*) only supports computing the entire tag in one go (the `poly1305_auth` function takes a key and message, and outputs the tag). However, the RFC8439 authenticated encryption (as used in BIP324, see #27634) scheme makes use of Poly1305 in a way where the message consists of 3 different pieces:
* The additionally authenticated data (AAD), padded to 16 bytes.
* The ciphertext, padded to 16 bytes.
* The length of the AAD and the length of the ciphertext, together another 16 bytes.
Implementing RFC8439 using the existing `poly1305_auth` function requires creating a temporary copy with all these pieces of data concatenated just for the purpose of computing the tag (the approach used in #25361).
This PR replaces the poly1305 code with new code from https://github.com/floodyberry/poly1305-donna (with minor adjustments to make it match our coding style and use our utility functions, documented in the commit) which supports incremental operation, and then adds a C++ wrapper interface using std::byte Spans around it, and adds tests that incremental and all-at-once computation match.
ACKs for top commit:
achow101:
ACK 4e5c933f6a
theStack:
ACK 4e5c933f6a
stratospher:
tested ACK 4e5c933.
Tree-SHA512: df6e9a2a4a38a480f9e4360d3e3def5311673a727a4a85b008a084cf6843b260dc82cec7c73e1cecaaccbf10f3521a0ae7dba388b65d0b086770f7fbc5223e2a
fa6dfaaf45 scripted-diff: Use new FUZZ_TARGET macro everywhere (MarcoFalke)
fa36ad8b09 fuzz: Accept options in FUZZ_TARGET macro (MarcoFalke)
Pull request description:
The `FUZZ_TARGET` macros have many issues:
* The developer will have to pick the right macro to pass the wanted option.
* Adding a new option requires doubling the number of existing macros in the worst case.
Fix all issues by using only a single macro.
This refactor does not change behavior.
ACKs for top commit:
dergoegge:
ACK fa6dfaaf45
Tree-SHA512: 49a34553867a1734ce89e616b2d7c29b784a67cd8990db6573f0c7b18957636ef0c81d3d0d444a04c12cdc98bc4c4aa7a2ec94e6232dc363620a746e28416444
4da243ba02 qt: show own outputs on PSBT signing window (Hernan Marino)
Pull request description:
This fixes https://github.com/bitcoin-core/gui/issues/732 .
It allows you to identify your own addresses in the outputs of a transaction in the PSBT signing window. This enables easy identification of change outputs, and prevents certain attacks where someone (co-signers of a multisig, or others ) might trick you into signing a transaction while they are stealing the change, since prior to this modification there was no easy way of knowing this.
The identification of the output is similar to the way this is done in the transaction details window.
A sample output is :
ACKs for top commit:
achow101:
ACK 4da243ba02
jarolrod:
ACK 4da243ba02
Tree-SHA512: fa9901d2acc84472c11afcd0a59a859db598cdf5cea755b492178d3e7434b70d9bd8f554928938a2ff9920c8f397fef814ce14b416556c30fba0c3c1f62cd722
31eca93a9e kernel: Remove StartShutdown calls from validation code (Ryan Ofsky)
Pull request description:
This change drops the last kernel dependency on shutdown.cpp. It also adds new hooks for libbitcoinkernel applications to be able to interrupt kernel operations when the chain tip changes.
This change is mostly a refactoring, but does slightly change `-stopatheight` behavior (see release note and commit message)
ACKs for top commit:
TheCharlatan:
ACK 31eca93a9e
furszy:
Concept and light review ACK 31eca93a
hebasto:
ACK 31eca93a9e, I have reviewed the code and it looks OK.
MarcoFalke:
lgtm ACK 31eca93a9e 🕷
Tree-SHA512: e26928436bcde658e842b1f92e9c24b1ce91031fb63b41aafccf3130bfff532b75338a269a2bb7558bff2973913f17b97a00fec3e7e0588e2ce44de097142047
35a2175ad8 fuzz: addrman, add coverage for `network` field in `Select()`, `Size()` and `GetAddr()` (brunoerg)
Pull request description:
This PR adds fuzz coverage for `network` field in `Select()`, `Size()` and `GetAddr()`, there was only call to them without passing a network.
https://marcofalke.github.io/b-c-cov/fuzz.coverage/src/addrman.cpp.gcov.html
ACKs for top commit:
amitiuttarwar:
for the record, ACK 35a2175ad8 - only small changes from the version (previously) proposed in 27213
achow101:
ACK 35a2175ad8
mzumsande:
Code Review ACK 35a2175ad8, haven't tested this yet, but I will let the fuzzer run for a while now.
Tree-SHA512: dddb8322298d6c373c8e68d57538470b11825a9a310a355828c351d5c0b19ff6779d024a800e3ea90126d0c050e86f71fd22cd23d1a306c784cef0f82c45e3ca
e7cf8657e1 test: add unit test for local address advertising (Martin Zumsande)
f4754b9dfb net: restrict self-advertisements with privacy networks (Martin Zumsande)
e4d541c7cf net, refactor: pass reference for peer address in GetReachabilityFrom (Martin Zumsande)
62d73f5370 net, refactor: pass CNode instead of CNetAddr to GetLocalAddress (Martin Zumsande)
Pull request description:
The current logic for self-advertisements works such that we detect as many local addresses as we can, and then, using the scoring matrix from `CNetAddr::GetReachabilityFrom()`, self-advertise with the address that fits best to our peer.
It is in general not hard for our peers to distinguish our self-advertisements from other addrs we send them, because we self-advertise every ~24h and because the first addr we send over a connection is likely our self-advertisement.
`GetReachabilityFrom()` currently only takes into account actual reachability, but not whether we'd _want_ to announce our identity for one network to peers from other networks, which is not straightforward in connection with privacy networks.
While the general approach is to prefer self-advertising with the address for the network our peer is on, there are several special situations in which we don't have one, and as a result could allow self-advertise other local addresses, for example:
A) We run i2p and clearnet, use `-i2pacceptincoming=0` (so we have no local i2p address), and we have a local ipv4 address. In this case, we'd advertise the ipv4 address to our outbound i2p peers.
B) Our `-discover` logic cannot detect any local clearnet addresses in our network environment, but we are actually reachable over clearnet. If we ran bitcoind clearnet-only, we'd always advertise the address our peer sees us with instead, and could get inbound peers this way. Now, if we also have an onion service running (but aren't using tor as a proxy for clearnet connections), we could advertise our onion address to clearnet peers, so that they would be able to connect our clearnet and onion identities.
This PR tries to avoid these situations by
1.) never advertising our local Tor or I2P address to peers from other networks.
2.) never advertising local addresses from non-anonymity networks to peers from Tor or I2P
Note that this affects only our own self-advertisements, the rules to forward other people's addrs are not changed.
[Edit] after Initial [discussion](https://github.com/bitcoin/bitcoin/pull/27411#issuecomment-1497176155): CJDNS is not being treated like Tor and I2P at least for now, because it has different privacy properties and for the practical reason that it has still very few bitcoin nodes.
ACKs for top commit:
achow101:
ACK e7cf8657e1
vasild:
ACK e7cf8657e1
luke-jr:
utACK e7cf8657e1
Tree-SHA512: 3db8415dea6f82223d11a23bd6cbb3b8cf68831321280e926034a1f110cbe22562570013925f6fa20d8f08e41d0202fd69c733d9f16217318a660d2a1a21b795
This also removes the old poly1305_auth interface, as it no longer serves any
function. The new Poly1305 class based interface is more modern and safe.
This code is taken from poly1305-donna-32.h, poly1305-donna.h, poly1305-donna.c
from https://github.com/floodyberry/poly1305-donna, commit
e6ad6e091d30d7f4ec2d4f978be1fcfcbce72781, with the following modifications:
* Coding style (braces around one-line indented if/for loops).
* Rename unsigned long (long) to uint32_t and uint64_t.
* Rename poly1305_block_size to POLY1305_BLOCK_SIZE.
* Adding noexcept to functions.
* Merging poly1305_state_internal_t and poly1305_context types.
* Merging code from multiple files.
* Place all imported code in the poly1305_donna namespace.
0bf87476f5 test: add ChaCha20 test triggering 32-bit block counter overflow (Sebastian Falbesoner)
7f2a985147 tests: improve ChaCha20 unit tests (Pieter Wuille)
511a8d406e crypto: Implement RFC8439-compatible variant of ChaCha20 (Pieter Wuille)
Pull request description:
Based on and replaces part of #25361, part of the BIP324 project (#27634). See also #19225 for background.
There are two variants of ChaCha20 in use. The currently implemented one uses a 64-bit nonce and a 64-bit block counter, while the one used in RFC8439 (and thus BIP324) uses a 96-bit nonce and 32-bit block counter. This PR changes the logic to use the 96-bit nonce variant, though in a way that's compatible with >256 GiB output (by automatically incrementing the first 32-bit part of the nonce when the block counter overflows).
For those who reviewed the original PR, the biggest change is here that the 96-bit nonce is passed as a Nonce96 type (pair of 32-bit + 64-bit integer) rather than a 12-byte array.
ACKs for top commit:
achow101:
ACK 0bf87476f5
theStack:
Code-review ACK 0bf87476f5
Tree-SHA512: 62e4cbd5388b8d50ef1a0dc99b6f4ad36c7b4419032035f8e622dda63a62311dd923032217e20054bcd836865d4be5c074f9e5538ca158f94f08eab75c5519c1
With C++11 (and later), the duplicate variable is no longer needed.
Also, run clang-format on the namespace, as the script in the next
commit relies on a specific format. This prevents a clang-format run in
the future from breaking the script. (Review hint: --ignore-all-space).
This change drops the last kernel dependency on shutdown.cpp. It also adds new
hooks for libbitcoinkernel applications to be able to interrupt kernel
operations when the chain tip changes.
This is a refactoring that does not affect behavior. (Looking at the code it
can appear like the new break statement in the ActivateBestChain function is a
change in behavior, but actually the previous StartShutdown call was indirectly
triggering a break before, because it was causing m_chainman.m_interrupt to be
true. The new code just makes the break more obvious.)
89ba8905f5 test: indexes, fix on error infinite loop (furszy)
Pull request description:
Coming from https://github.com/bitcoin/bitcoin/pull/28036#issuecomment-1623813703, I thought that we were going to fix it there but seems that got merged without it for some reason.
As index sync failures trigger a shutdown request without notifying `BaseIndex::BlockUntilSyncedToCurrentChain` in any way, we also need to check whether a shutdown was requested or not inside 'IndexWaitSynced'.
Otherwise, any error inside the index sync process will hang the test forever.
ACKs for top commit:
MarcoFalke:
lgtm ACK 89ba8905f5
jamesob:
ACK 89ba890
ryanofsky:
Code review ACK 89ba8905f5. Just comment update since last review
Tree-SHA512: 1f6daf34e51d3fbc802799bfa4ac0ef0d8f774db5f9e2f5d35df18a77679778475c94efc3da1fb723ebaf3583e4075e4a5cbe4a5104ad0c50e2b32076e247b29
The `UNKNOWN_DESCRIPTOR` error comes from the
`WalletDescriptor::DeserializeDescriptor` std::ios_base
exception, which contains further information about the
parsing error.
This has the benefit of moving the StartShutdown call out of the
blockstorage file and thus out of the kernel's responsibility. The user
can now decide if he wants to start shutdown / interrupt after a block
import or not.
As index sync failures trigger a shutdown request without notifying
BaseIndex::BlockUntilSyncedToCurrentChain in any way, we also need
to check whether a shutdown was requested or not inside 'IndexWaitSynced'.
Otherwise, any error inside the index sync process will hang the test
forever.
ca91c244ef index: verify blocks data existence only once (furszy)
fcbdaeef4d init: don't start indexes sync thread prematurely (furszy)
2ec89f1970 refactor: simplify pruning violation check (furszy)
c82ef91eae make GetFirstStoredBlock assert that 'start_block' always has data (furszy)
430e7027a1 refactor: index, decouple 'Init' from 'Start' (furszy)
225e213110 refactor: init indexes, decouple 'Start()' from the creation step (furszy)
2ebc7e68cc doc: describe 'init load' thread actions (Martin Zumsande)
04575106b2 scripted-diff: rename 'loadblk' thread name to 'initload' (furszy)
ed4462cc78 init: start indexes sync earlier (furszy)
Pull request description:
Simplifies index startup code, eliminating the `g_indexes_ready_to_sync` variable,
deduplicating code and moving the prune violation check out of the `BaseIndex` class.
Also makes startup more efficient by running the prune violation check once for all indexes
instead of once for each index, and by delaying the prune violation check and moving it off
of the main thread so the node can start up faster and perform the block data availability
verification even when the '-reindex" or the "-reindex-chainstate" flags are enabled (which
hasn't being possible so far).
ACKs for top commit:
ryanofsky:
Code review ACK ca91c244ef. Just rebase and suggested changes since last review (Start return check, and code simplification)
TheCharlatan:
re-ACK ca91c244ef
Tree-SHA512: e9c98ce89aeb29e8d0f505f17b34aa54fe44efefbf017f4746e3b446ab4de25ade4f707254a0bbe4b99b69731b04a4067ce529eb7aa834ced196784b694cf7ce
At present, during init, we traverse the chain (once per index)
to confirm that all necessary blocks to sync each index up to
the current tip are present.
To make the process more efficient, we can fetch the oldest block
from the indexers and perform the chain data existence check from
that point only once.
This also moves the pruning violation check to the end of the
'loadinit' thread, which is where the reindex, block loading and
chain activation processes happen.
Making the node's startup process faster, allowing us to remove
the global g_indexes_ready_to_sync flag, and enabling the
execution of the pruning violation verification even when the
reindex or reindex-chainstate flags are enabled (which has being
skipped so far).
By moving the 'StartIndexes()' call into the 'initload'
thread, we can remove the threads active wait. Optimizing
the available resources.
The only difference with the current state is that now the
indexes threads will only be started when they can process
work and not before it.
By generalizing 'GetFirstStoredBlock' and implementing
'CheckBlockDataAvailability' we can dedup code and
avoid repeating work when multiple indexes are enabled.
E.g. get the oldest block across all indexes and
perform the pruning violation check from that point
up to the tip only once (this feature is being introduced
in a follow-up commit).
This commit shouldn't change behavior in any way.
Co-authored-by: Ryan Ofsky <ryan@ofsky.org>
And transfer the responsibility of verifying whether 'start_block'
has data or not to the caller.
This is because the 'GetFirstStoredBlock' function responsibility
is to return the first block containing data. And the current
implementation can return 'start_block' when it has no data!. Which
is misleading at least.
Edge case behavior change:
Previously, if the block tip lacked data but all preceding blocks
contained data, there was no prune violation. And now, such
scenario will result in a prune violation.
