fafbc70d48e1fb42c4ed3da609e5f30c4cc39418 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 fafbc70d48e1fb42c4ed3da609e5f30c4cc39418
achow101:
ACK fafbc70d48e1fb42c4ed3da609e5f30c4cc39418
rkrux:
utACK fafbc70d48e1fb42c4ed3da609e5f30c4cc39418
theStack:
ACK fafbc70d48e1fb42c4ed3da609e5f30c4cc39418
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.
1808b5aaf7c4910122fcd088e03d790189907438 clusterlin: remove unused FixLinearization (cleanup) (Pieter Wuille)
34a77138b7a6f65a31c6273f2770e943cc0b474b txgraph: permit non-topological clusters to defer fixing (optimization) (Pieter Wuille)
3380e0cbb59b2a893faabfa8f9aa18e582a11c2f txgraph: use PostLinearize less prior to linearizing (Pieter Wuille)
62dd88624a7fba14d2dac32d94fd101eb378d1e7 txgraph: drop NEEDS_SPLIT_ACCEPTABLE (simplification) (Pieter Wuille)
01ffcf464a4679136185e323b5b0328695a5bd1e 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 1808b5aaf7c4910122fcd088e03d790189907438
marcofleon:
code review ACK 1808b5aaf7c4910122fcd088e03d790189907438
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.
5b7bf47f9b92f5d4483a69d0682797e8cf483434 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 5b7bf47f9b92f5d4483a69d0682797e8cf483434
billymcbip:
ACK 5b7bf47f9b92f5d4483a69d0682797e8cf483434
maflcko:
lgtm ACK 5b7bf47f9b92f5d4483a69d0682797e8cf483434
Tree-SHA512: b895873b82f904c2ee9a81b4a2fbb365b60c57f04587ded5ddc7907d209520acb6073f5dd1a19cb2ae6aadab3c85a5ac751c8c398ce7c0e29314eea54e61295c
31852057ea393e64b7ea2bfabc75a55fda40b786 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 31852057ea393e64b7ea2bfabc75a55fda40b786
maflcko:
lgtm ACK 31852057ea393e64b7ea2bfabc75a55fda40b786
w0xlt:
ACK 31852057ea
naiyoma:
utACK 31852057ea393e64b7ea2bfabc75a55fda40b786
Tree-SHA512: 24696f6005c7131d4c9328f6ff43ddded863b8ba6b2cac6f6009bcb4617616c0c35a0b55812d5010f74385d8e6d4ea09dd2b06b5f4ada2bb7e86d7abee764192
fa65bc0e79dab8a97e0e3f9f31540f1c029a2f6e test: Run bench sanity checks in parallel with functional tests (MarcoFalke)
fa9fdbce7928e1ce8c5e17d965ff9bcc1282c8f5 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 fa65bc0e79dab8a97e0e3f9f31540f1c029a2f6e
l0rinc:
Tested ACK fa65bc0e79dab8a97e0e3f9f31540f1c029a2f6e
janb84:
tACK fa65bc0e79dab8a97e0e3f9f31540f1c029a2f6e
willcl-ark:
ACK fa65bc0e79dab8a97e0e3f9f31540f1c029a2f6e
Tree-SHA512: d27e363b7896a7543a4ee8df41a56e58b74f07d4f296e2e5ee293fc91817d0be310e26905755fb94d44417d94fa29ad4cc5d4aa19e78d25d41bc2d9e0948c034
4ce3f4a26565e9851af950728cda7bcc2242d0c2 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 4ce3f4a26565e9851af950728cda7bcc2242d0c2
achow101:
ACK 4ce3f4a26565e9851af950728cda7bcc2242d0c2
fjahr:
utACK 4ce3f4a26565e9851af950728cda7bcc2242d0c2
rkrux:
crACK 4ce3f4a26565e9851af950728cda7bcc2242d0c2
janb84:
cr ACK 4ce3f4a26565e9851af950728cda7bcc2242d0c2
Tree-SHA512: b296a28d30084fd35c67a2162e85576e3365e5d6fffe5b1add500034c1850604ee8c37b61afe812bfab8a7cc20f6a9e22db445e3c371311a5f82a777e5700ebf
5805a8b54083539c4fede52b4b726102dcfc864e 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 5805a8b54083539c4fede52b4b726102dcfc864e
achow101:
ACK 5805a8b54083539c4fede52b4b726102dcfc864e
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.
fac5a1b10a6979a7898c5c3555d62b593560772f 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 fac5a1b10a6979a7898c5c3555d62b593560772f
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.
fab300b378941a233119805c0d62198596a57790 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 fab300b378941a233119805c0d62198596a57790
l0rinc:
ACK fab300b378941a233119805c0d62198596a57790
rkrux:
lgtm crACK fab300b378941a233119805c0d62198596a57790
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.
Since #28280, the cost of a non-wiping sync of the UTXO cache is only proportional to the number of dirty entries, rather than proportional to the size of the entire cache. Because of that, there is no reason to perform a wiping flush in case the contents of the cache is still useful.
Split the FlushStateMode::ALWAYS mode into a FORCE_SYNC (non-wiping) and a FORCE_FLUSH (wiping), and then use the former in scantxoutset, gettxoutsetinfo, snapshot creation.
Co-authored-by: l0rinc <pap.lorinc@gmail.com>
Co-authored-by: cedwies <141683552+cedwies@users.noreply.github.com>
This prepares the addition of `FORCE_SYNC`.
`empty_cache` in `FlushStateToDisk` was moved up to be reusable and `FlushStateMode::FORCE_FLUSH` was used as a placeholder before we properly split the two new states.
`log_utxocache_flush.py` was regenerated and the alignment adjusted for the wider `FlushStateMode` values.
Co-authored-by: Pieter Wuille <pieter@wuille.net>
Co-authored-by: optout <13562139+optout21@users.noreply.github.com>
6da6f503a6dd8de85780ca402f5202095aa8b6ea refactor: Let CCoinsViewCache::BatchWrite return void (TheCharlatan)
Pull request description:
CCoinsViewCache::BatchWrite always returns true if called from a backed cache, so just return void instead. Also return void from ::Sync and ::Flush.
This allows for dropping a FatalError condition and simplifying some dead error handling code a bit.
Since we now no longer exercise the "error path" when returning from `CCoinsView::BatchWrite`, make the method clear the cache instead. This should only be exercised by tests and not change production behaviour. This might slightly improve the coins_view fuzz test's ability to generate better coverage.
ACKs for top commit:
l0rinc:
ACK 6da6f503a6dd8de85780ca402f5202095aa8b6ea
andrewtoth:
re-ACK 6da6f503a6dd8de85780ca402f5202095aa8b6ea
achow101:
ACK 6da6f503a6dd8de85780ca402f5202095aa8b6ea
w0xlt:
ACK 6da6f503a6
Tree-SHA512: dfaa325b0cf8108910aebf1b27434aaddb639d10d860e96797c77ea42eca9035a54a7dc1d6a5d4eae2b75fcc9356206d3d5672243d2c906e80d19024c8b95408
76c092ff805833a9adf84f669f0455bc2e0bba8b wallet: warn against accidental unsafe older() import (Sjors Provoost)
592157b7594693da389e4bd9b2cdedbdba7556fc test: move SEQUENCE_LOCKTIME flags to script (Sjors Provoost)
Pull request description:
[BIP 379](https://github.com/bitcoin/bips/blob/master/bip-0379.md) ([Miniscript](https://bitcoin.sipa.be/miniscript/)) allows relative height and time locks that have no consensus meaning in [BIP 68](https://github.com/bitcoin/bips/blob/master/bip-0068.mediawiki) (relative timelocks) / [BIP 112](https://github.com/bitcoin/bips/blob/master/bip-0112.mediawiki) (`CHECKSEQUENCEVERIFY`). This is (ab)used by some protocols, e.g. [by Lightning to encode extra data](https://delvingbitcoin.org/t/exploring-extended-relative-timelocks/1818/23), but is unsafe when used unintentionally: `older(65536)` is equivalent to `older(1)`.
This PR emits a warning when `importdescriptors` contains such a descriptor.
The first commit makes `SEQUENCE_LOCKTIME` flags reusable by other tests.
The main commit adds the `ForEachNode` helper to `miniscript.h` which is then used in the `MiniscriptDescriptor` constructor to check for `Fragment::OLDER` with unsafe values. These are stored in `m_warnings`, which the RPC code then collects via `Warnings()`.
It adds both a unit and functional test.
---
A previous version of this PR prevented the import, unless the user opted in with an `unsafe` flag. It also used string parsing in the RPC code.
---
Based on:
- [x] https://github.com/bitcoin/bitcoin/pull/33914
ACKs for top commit:
pythcoiner:
reACK 76c092ff80
achow101:
ACK 76c092ff805833a9adf84f669f0455bc2e0bba8b
rkrux:
lgtm re-ACK 76c092ff805833a9adf84f669f0455bc2e0bba8b
brunoerg:
reACK 76c092ff805833a9adf84f669f0455bc2e0bba8b
Tree-SHA512: 8e944e499bd4a43cc27eeb889f262b499b9b07aa07610f4a415ccb4e34a9110f9946646f446a54ac5bf17494d8d96a89e4a1fa278385db9b950468f27283e17a
