- Refactor Descriptor::ToPrivateString() to allow descriptors with
missing private keys to be printed. Useful in descriptors with
multiple keys e.g tr() etc.
- The existing behaviour of listdescriptors is preserved as much as
possible, if no private keys are availablle ToPrivateString will
return false
This commit modifies the Pubkey providers to return the public string
if private data is not available.
This is setup for a future commit to make Descriptor::ToPrivateString
return strings with missing private key information.
Co-authored-by: rkrux <rkrux.connect@gmail.com>
ToPrivateString() behaviour will be modified in the following commits.
In order to keep the scope of this PR limited to the RPC behaviour,
this commit updates wallet migration to use 'Descriptor::HavePrivateKeys()'
in place of 'Descriptor::ToPrivateString()' to determine watchonly descriptors.
A follow-up PR can be opened to update migration logic to exclude
descriptors with some private keys from the watchonly migration wallet.
Previously, to determine if a desc is watchonly, `ToPrivateString()`, was used.
It returns `false` if there is at least one pubkey in the descriptor for which
the provider does not have a private key.
ToPrivateString() behaviour will change in the following commits to only
return `false` if no priv keys could be found for the pub keys in the descriptor.
HavePrivateKeys() is added here to replace the use of ToPrivateString() for determining
if a descriptor is 'watchonly'.
Co-authored-by: rkrux <rkrux.connect@gmail.com>
fafbc70d48 rpc: [wallet] Use unsigned type for tx version in sendall (MarcoFalke)
Pull request description:
It is confusing to parse the unsigned tx version as a signed type. Also, it makes it harder to use the integer sanitizer.
Can be tested via:
* Build with the flags `-DCMAKE_C_COMPILER='clang' -DCMAKE_CXX_COMPILER='clang++' -DSANITIZERS=undefined,integer,float-divide-by-zero`
* Set the existing suppressions: `export UBSAN_OPTIONS="suppressions=$(pwd)/test/sanitizer_suppressions/ubsan:print_stacktrace=1:halt_on_error=0:report_error_type=1"`
* Start the RPC server, e.g. `./bld-cmake/bin/bitcoin-qt -datadir=/tmp -regtest -server`
* Call the sendall RPC, e.g. `./bld-cmake/bin/bitcoin-cli -datadir=/tmp -regtest -named sendall '["bcrt1qlrt3xps4wxpfcjmljrayr2ualczmnfvd4vzdq3"]' fee_rate=1.234 version=-1`
Before:
```
src/wallet/rpc/spend.cpp:1470:42: runtime error: implicit conversion from type 'int' of value -1 (32-bit, signed) to type 'uint32_t' (aka 'unsigned int') changed the value to 4294967295 (32-bit, unsigned)
Invalid parameter, version out of range(1~3)
```
After:
```
JSON integer out of range
ACKs for top commit:
bensig:
ACK fafbc70d48
achow101:
ACK fafbc70d48
rkrux:
utACK fafbc70d48
theStack:
ACK fafbc70d48
Tree-SHA512: bb7cf54e9691ad2591646b138ffdfac95bf77c5234d489f4e4f2c60b41bdc14cdc18a030fecb0a6ac64e55e4c69b37835afd334f87d8a44b8df6cda053e8fefb
Because the default wallet has no name, the watch-only and solvables
wallets created during migration end up having no name either.
This fixes it by applying the same prefix name we use for the backup
file for an unnamed default wallet.
Before: watch-only wallet named "_watchonly"
After: watch-only wallet named "default_wallet_watchonly"
Right now, after migration the last message users see is "migration completed",
but the migration isn't actually finished yet. We still need to load the new wallets
to ensure consistency, and if that fails, the migration will be rolled back. This
can be confusing for users.
This change logs the post-migration loading step and if a wallet fails to load and
the migration will be rolled back.
The first test verifies that restoring into an existing empty directory
or a directory with no .dat db files succeeds, while restoring into a
dir with a .dat file fails.
The second test covers restoring into the default unnamed wallet
(wallet.dat), which also implicitly exercises the recovery path used
after a failed migration.
The third test covers failure during restore on a prune node. When
the wallet last sync was beyond the pruning height.
When migrating any legacy unnamed wallet, a failed migration would
cause the cleanup logic to remove its parent directory. Since this
type of legacy wallet lives directly in the main '/wallets/' folder,
this resulted in unintentionally erasing all wallets, including the
backup file.
To be fully safe, we will no longer call `fs::remove_all`. Instead,
we only erase the individual db files we have created, leaving
everything else intact. The created wallets parent directories are
erased only if they are empty.
As part of this last change, `RestoreWallet` was modified to allow
an existing directory as the destination, since we no longer remove
the original wallet directory (we only remove the files we created
inside it). This also fixes the restore of top-level default wallets
during failures, which were failing due to the directory existence
check that always returns true for the /wallets/ directory.
This bug started after:
f6ee59b6e2
Previously, the `fs::copy_file` call was failing for top-level wallets,
which prevented the `fs::remove_all` call from being reached.
Track what RestoreWallet creates so only those files and directories
are removed during a failure and nothing else. Preexisting paths
must be left untouched.
Note:
Using fs::remove_all() instead of fs::remove() in RestoreWallet does
not cause any problems currently, but the change is necessary for the
next commit which extends RestoreWallet to work with existing directories,
which may contain files that must not be deleted.
1808b5aaf7 clusterlin: remove unused FixLinearization (cleanup) (Pieter Wuille)
34a77138b7 txgraph: permit non-topological clusters to defer fixing (optimization) (Pieter Wuille)
3380e0cbb5 txgraph: use PostLinearize less prior to linearizing (Pieter Wuille)
62dd88624a txgraph: drop NEEDS_SPLIT_ACCEPTABLE (simplification) (Pieter Wuille)
01ffcf464a clusterlin: support fixing linearizations (feature) (Pieter Wuille)
Pull request description:
Part of #30289, follow-up to #32545.
This gets rid of `FixLinearization()` by integrating the functionality into `Linearize()`, and makes txgraph exploit that (by delaying fixing of clusters until their first re-linearization). It also reduces (but does not eliminate) the number of calls to `PostLinearize`, as the SFL linearization effectively performs something very similar to postlinearization when loading in an existing linearization already.
ACKs for top commit:
instagibbs:
reACK 1808b5aaf7
marcofleon:
code review ACK 1808b5aaf7
Tree-SHA512: 81cd9549de2968f5126079cbb532e2cb052ea8157c9c9ce37fd39ad2294105d7c79ee8d946c3d8f7af5b2119299a232c448b42a33e1e43ccc778a5b52957e387
`CoinsViewBottom` roughly simulates a memory-backed `CCoinsViewDB`, which never stores spent coins.
Stop returning spent coins from `GetCoin()`, erase spent entries in `BatchWrite()`, and tighten comparisons to expect `std::nullopt` when the simulator has no coin.
Co-authored-by: Lőrinc <pap.lorinc@gmail.com>
Production `GetCoin()` implementations only return unspent coins.
Update the `CCoinsView` test backend to match that contract, so tests stop exercising cache states that cannot occur with `CCoinsViewCache` or `CCoinsViewDB`.
Co-authored-by: Lőrinc <pap.lorinc@gmail.com>
The chainstate UTXO database only stores unspent outputs; spent entries are removed.
Assert after reading a `Coin` so corruption or misuse cannot propagate a spent coin through the `GetCoin()` interface.
5b7bf47f9b doc: p2p: replace last remaining "command" terminology with "message type" (Sebastian Falbesoner)
Pull request description:
This small PR is (presumably) the final one in a long series of replacing the confusing "command" terminology with "message type" when referring to the header field of P2P messages, see #18533, #18937, #24078, #24141 and #31163.
The instances were found manually via `$ git grep -i command`, hope I didn't miss any.
ACKs for top commit:
l0rinc:
ACK 5b7bf47f9b
billymcbip:
ACK 5b7bf47f9b
maflcko:
lgtm ACK 5b7bf47f9b
Tree-SHA512: b895873b82f904c2ee9a81b4a2fbb365b60c57f04587ded5ddc7907d209520acb6073f5dd1a19cb2ae6aadab3c85a5ac751c8c398ce7c0e29314eea54e61295c
31852057ea test: fix intermittent failure in p2p_addr_selfannouncement (0xb10c)
Pull request description:
Due to the mocktime being bumped before the expected time is updated, it could happen that the self-announcement is send with an newer timestamp than what we expect. To fix this, update the expected time before we bump the mocktime.
closes#34159
ACKs for top commit:
bensig:
ACK 31852057ea
maflcko:
lgtm ACK 31852057ea
w0xlt:
ACK 31852057ea
naiyoma:
utACK 31852057ea
Tree-SHA512: 24696f6005c7131d4c9328f6ff43ddded863b8ba6b2cac6f6009bcb4617616c0c35a0b55812d5010f74385d8e6d4ea09dd2b06b5f4ada2bb7e86d7abee764192
fa65bc0e79 test: Run bench sanity checks in parallel with functional tests (MarcoFalke)
fa9fdbce79 test: Pass bench exe into test framework utils (MarcoFalke)
Pull request description:
The ctest target `bench_sanity_check` has many issues:
* With sanitizers enabled, it is one of the slowest targets, often taking several minutes. See https://github.com/bitcoin/bitcoin/issues/32770#issuecomment-2984264066.
* There is no insight from ctest into how long each individual sanity check takes.
* On a timeout, or OOM issue, there is no insight into which sub-bench failed. The failure will generally just look like `75/153 Test #9: bench_sanity_check ...................***Failed 770.84 sec out of memory`
* Places that can't use ctest (like the Windows-cross CI task) have to explicitly run it, or risk forgetting to run it.
* All benchmarks are run sequentially, when they could run in parallel instead.
Both issues can lead to CI timeouts and leave CPU unused during testing.
Fix all issues by running it as part of the functional tests instead. This is similar to the rpcauth tests (https://github.com/bitcoin/bitcoin/pull/32881) and util tests [bitcoin-tx, and bitcoin-util] (https://github.com/bitcoin/bitcoin/pull/32697).
ACKs for top commit:
achow101:
ACK fa65bc0e79
l0rinc:
Tested ACK fa65bc0e79
janb84:
tACK fa65bc0e79
willcl-ark:
ACK fa65bc0e79
Tree-SHA512: d27e363b7896a7543a4ee8df41a56e58b74f07d4f296e2e5ee293fc91817d0be310e26905755fb94d44417d94fa29ad4cc5d4aa19e78d25d41bc2d9e0948c034
4ce3f4a265 rpc, net: deprecate `startingheight` field of `getpeerinfo` RPC (Sebastian Falbesoner)
Pull request description:
This PR deprecates the "startingheight" result field of the `getpeerinfo` RPC, following the discussion in #33990.
Rationale: the reported starting height of a peer in the VERSION message is untrusted, and it doesn't seem to be useful anymore (after #20624), so deprecating the corresponding field seems reasonable. After that, it can be removed, along with the `m_starting_height` field of the Peer / CNodeStats structs, as it is sufficient to show the reported height only once at connection in the debug log.
ACKs for top commit:
optout21:
crACK 4ce3f4a265
achow101:
ACK 4ce3f4a265
fjahr:
utACK 4ce3f4a265
rkrux:
crACK 4ce3f4a265
janb84:
cr ACK 4ce3f4a265
Tree-SHA512: b296a28d30084fd35c67a2162e85576e3365e5d6fffe5b1add500034c1850604ee8c37b61afe812bfab8a7cc20f6a9e22db445e3c371311a5f82a777e5700ebf
5805a8b540 psbt: detect invalid MuSig2 pubkeys in deserialization (rkrux)
Pull request description:
Throw error while deserializing PSBT if invalid pubkeys are passed
as a MuSig2 aggregate or participant.
Should fix#33999 & #34201 by throwing error at the very start while decoding
an invalid PSBT that should subsequently not allow the MuSig2
signing operation to take place, thereby avoiding the crash.
ACKs for top commit:
fjahr:
utACK 5805a8b540
achow101:
ACK 5805a8b540
Tree-SHA512: 4741db96b278e9f3d532e1873af9530a70bbc7a8d3625b9e1c07001acc472fc10cbb79995c16bc4d06cc568ef98fe8d2b8e8d87b617dc05d7554085ffb92dfef
With the new SFL algorithm, the process of loading an existing linearization into the
SFL state is very similar to what PostLinearize does. This means there is little benefit
to performing an explicit PostLinearize step before linearizing inside txgraph. Instead,
it seems better to use our allotted CPU time to perform more SFL optimization steps.
With the SFL algorithm, we will practically be capable of keeping
most if not all clusters optimal. With that, it seems less valuable
to avoid doing work after splitting an acceptable cluster, because by
doing some work we may get it to OPTIMAL.
This reduces the complexity of the code a bit as well.
fac5a1b10a test: Allow mempool_updatefromblock.py to run on 32-bit (MarcoFalke)
Pull request description:
The number of dropped parent transactions in the `test_max_disconnect_pool_bytes` test was hard-coded to `2`.
This happens to work fine on 64-bit for now. However, it seems to fail on 32-bit (https://github.com/bitcoin/bitcoin/issues/34108).
I don't think we care about the exact number, as long as it is at least `1`.
So hard-code `1` for an initial sanity check, and then calculate the exact value at runtime via `len(mempool) // 2`.
Also, enable the functional tests in 32-bit CI, to confirm the regression test.
Fixes https://github.com/bitcoin/bitcoin/issues/34108
ACKs for top commit:
bensig:
ACK fac5a1b10a
instagibbs:
ACK fac5a1b10a
Tree-SHA512: 8d468f306d95e52cbfac1803293e3b8e9575c9010200010c7833382112509e0d51827dc9681b0b68eeae742af2c14d12da5fd4cf0e1d871a02f91fc80e6720d1
This makes sure the initial address self-announcement a node sends to
a peer happends in a separate P2P message. This has benefits for both
inbound and outbound connections:
For inbound connections from a peer to us, previously, we might send
the self-announcement along with our response to a GETADDR request.
However, the self-announcement might replace an address from the
GETADDR response. This isn't clean.
For outbound connections from us to a peer, previously, it could have
happend that we send the self-announcement along with other addresses.
Since shortly after connection open, the peer might only have one
rate-limiting token for us, and the addresses are shuffeld on arrival,
it's possible that the self-announcement gets rate-limited. However,
note that these rate-limitings seem to be rare in practice.
This is inspired by and based on https://github.com/bitcoin/bitcoin/pull/33699#issuecomment-3462287763
Co-Authored-By: Anthony Towns <aj@erisian.com.au>
Due to the mocktime being bumped before the expected time is updated,
it could happen that the self-announcement is send with an newer
timestamp than what we expect. To fix this, update the expected time
before we bump the mocktime.
closes#34159
Introduce a new method intended to replace getCoinbaseRawTx(), which
provides a struct with everything clients need to construct a coinbase.
This is safer than providing a raw dummy coinbase that clients then have
to manipulate.
The CoinbaseTx data is populated during the dummy transaction generation
and stored in struct CBlockTemplate.
Expand the interface_ipc.py functional test to document its usage
and ensure equivalence.
fab300b378 test: Enable ruff E713 lint (MarcoFalke)
Pull request description:
Membership tests of the form `not item in stuff` may be confusing, because they could be read as `(not item) in stuff`, which is different.
So enable the ruff E713 lint, which should also help to avoid having to go through review cycles for this.
ACKs for top commit:
bensig:
ACK fab300b378
l0rinc:
ACK fab300b378
rkrux:
lgtm crACK fab300b378
Tree-SHA512: c3eaf0fbe0dd22d8e04b896e98adaf28162fb748e6f7f5ebfd73b2020da66046bf8f0c1a27db5da05250366b98ded8c4a55d53edd8fa050e80521aee42ba3c5a
The reported starting height of a peer in the VERSION message is
untrusted, and it doesn't seem to be useful anymore (after #20624),
so deprecating the corresponding "startingheight" field seems
reasonable. After that, it can be removed, along with the
`m_starting_height` field of the Peer / CNodeStats structs, as it is
sufficient to show the reported height only once at connection in the
debug log.
