fa9198af55df74b0c19c9125d256ad4df83cf005 lint: Check for missing trailing newline (MarcoFalke)
fa2b2aa27c29fe810e296ef82126553b8f0d56e6 lint: Add archived notes to default excludes (MarcoFalke)
Pull request description:
A missing trailing newline is harmless, but a bit problematic:
* `git` shows a warning by default
* After another line is appended, the diff will be verbose and `git blame` will be wrong for the "untouched" line.
Fix the problems by just requiring what is already the default, see also 663a9cabf8/.editorconfig (L9) and 663a9cabf8/test/lint/test_runner/src/main.rs (L327)
ACKs for top commit:
l0rinc:
utACK fa9198af55df74b0c19c9125d256ad4df83cf005
fanquake:
ACK fa9198af55df74b0c19c9125d256ad4df83cf005
Tree-SHA512: d144eebdeee68fc3404aa4a66ecd5c130f907ed4b869bd300f6e9ed74d125561d1f4cdd6dd20d9e969471a7d007399f928f072d1c1f626275ca31f32bc23fdbc
32023 changed AddWalletDescriptor to return util::Error, but did not
change all of the failure cases to do so. This may result in some
callers continuing when there was actually an error. Unify all of the
failure cases to use util::Error so that all callers handle
AddWalletDescriptor errors in the same way.
The encapsulated return type is changed from ScriptPubKeyMan* to
std::reference_wrapper<DescriptorScriptPubKeyMan>. This avoids having a
value that can be interpreted as a bool, and also removes the need to
constantly dynamic_cast the returned value. The only kind of
ScriptPubKeyMan that can come out of AddWalletDescriptor is a
DescriptorScriptPubKeyMan anyways.
30a94b1ab9ae850d55cb9eb606a06890437bc75e test, wallet: Remove concurrent writes test (Ava Chow)
b44b7c03fef01e0b5db704e50762b3d16b3da69e wallet: Write best block record on unload (Ava Chow)
876a2585a8b69e12ac171a0d9ff5aab864067c42 wallet: Remove unnecessary database Close step on shutdown (Ava Chow)
98a1a5275c8c395fe47ff7f10109d75b06bc391d wallet: Remove chainStateFlushed (Ava Chow)
7fd3e1cf0c88553e0722048ce488f265883558e7 wallet, bench: Write a bestblock record in WalletMigration (Ava Chow)
6d3a8b195a826448c021dd189255ca41ba70cc5a wallet: Replace chainStateFlushed in loading with SetLastBlockProcessed (Ava Chow)
7bacabb204b6c34f9545f0b37e2c66296ad2c0de wallet: Update best block record after block dis/connect (Ava Chow)
Pull request description:
Implements the idea discussed in https://github.com/bitcoin/bitcoin/pull/29652#issuecomment-2010579484
Currently, `m_last_block_processed` and `m_last_block_processed_height` are not guaranteed to match the block locator stored in the wallet, nor do either of those fields actually represent the last block that the wallet is synced up to. This is confusing and unintuitive.
This PR changes those last block fields to be updated whenever the wallet makes a change to the db for new transaction state found in new blocks. Whenever a block is received that contains a transaction relevant to the wallet, the last block locator will now be written to disk. Furthermore, every block disconnection will now write an updated locator.
To ensure that the locator is relatively recent and loading rescans are fairly quick in the event of unplanned shutdown, it is also now written every 144 blocks (~1 day). Additionally it is now written when the wallet is unloaded so that it is accurate when the wallet is loaded again.
Lastly, the `chainstateFlushed` notification in the wallet is changed to be a no-op. The best block locator record is no longer written when `chainstateFlushed` is received from the node since it should already be mostly up to date.
ACKs for top commit:
rkrux:
ACK 30a94b1ab9ae850d55cb9eb606a06890437bc75e
mzumsande:
Code Review ACK 30a94b1ab9ae850d55cb9eb606a06890437bc75e
ryanofsky:
Code review ACK 30a94b1ab9ae850d55cb9eb606a06890437bc75e. Only changes since last review are using WriteBestBlock method more places and updating comments.
Tree-SHA512: 46117541f8aaf13dde57430e813b4bbbd5e146e2632769675803c8e65a82f149a7cc6026489a127d32684b90124bd2b7c28216dbcfa6a47447300e8f3814e029
Descriptor wallets are already created by default since v23.0, but
since the recent legacy wallet removal this parameter *must* be True
(see commit 9f04e02ffaee0fe64027dc56c7bea3885254321a), i.e. still
passing it wouldn't contain any information for test readers
anymore. So simply drop them in the functional tests in order to
reduce code bloat.
-BEGIN VERIFY SCRIPT-
sed -i 's/, descriptors=True//g' $(git ls-files -- 'test/functional' ':(exclude)test/functional/wallet_backwards_compatibility.py')
sed -i '/descriptors=True,/d' ./test/functional/mempool_persist.py
-END VERIFY SCRIPT-
b104d442277090337ce405d92f1398b7cc9bcdb7 test: Remove RPCOverloadWrapper (Ava Chow)
4d32c19516fdb65b364638fa088b8d7167b438b6 test: Replace importpubkey (Ava Chow)
fe838dd391be669ccd0765b95f81c25fecfd3636 test: Replace usage of addmultisigaddress (Ava Chow)
d3142077794f4e910d3cdc749020d725e30feb24 test: Replace usage of importaddress (Ava Chow)
fcc457573f9b39e6f173a4f51c45d7dbb47e7ab0 test: Replace importprivkey with wallet_importprivkey (Ava Chow)
94c87bbbd06eb9a57930b9f59315533cfbe8f460 test: Remove unnecessary importprivkey from wallet_createwallet (Ava Chow)
Pull request description:
`RPCOverloadWrapper` implemented overloads for legacy wallet only RPCs so that the same function call could be used within tests for both legacy wallets and descriptor wallets. With legacy wallets now removed, there is no need to continue to have these overloads.
For `importaddress`, `addmultisigaddress`, and `importpubkey`, the uses of these are converted to `importdescriptors`.
For `importprivkey`, a new helper function `wallet_imporprivkey` is introduced that does what the overload did. This is mainly to reduce verbosity as `importprivkey` was more widely used throughout the tests.
Some tests that used these RPCs are now also no longer relevant and have been removed.
ACKs for top commit:
Sjors:
ACK b104d442277090337ce405d92f1398b7cc9bcdb7
pablomartin4btc:
cr ACK b104d442277090337ce405d92f1398b7cc9bcdb7
rkrux:
ACK b104d442277090337ce405d92f1398b7cc9bcdb7
w0xlt:
ACK b104d44227
Tree-SHA512: ded2f73829e2ce28466d4a9738eb382783ad990daee5d1859dbc4d354e6f8eec0c483ed5ecb1287fe0dd24ac332065b733a30d71b126b841bd7cd49e9a094b6d
When dealing with URI parts, it seems more consistent to use
corresponding SAFE_CHARS_URI mode in error messages.
Co-Authored-By: stickies-v <stickies-v@protonmail.com>
Currently the per-tx sigops limit standardness check (bounded by
`MAX_STANDARD_TX_SIGOPS_COST`, throwing "bad-txns-too-many-sigops"
if exceeded) is only indirectly tested with the much higher per-block
consensus limit (`MAX_BLOCK_SIGOPS_COST`), i.e. an increase in the
limit would still pass all tests. Refine that by splitting up the invalid
tx template `TooManySigops` in a per-block and a per-tx one.
The involved functional tests taking use of these templates are
`feature_block.py` and `p2p_invalid_txs.py`.
516f0689b511c09153e4b6b4a58dfedd61c6cda7 refactor: re-enable UBSan implicit-sign-change in serialize.h (Lőrinc)
5827e93507792dbbc33514d2c7c75a9ab7f7db89 refactor: use consistent size type for serialization template parameters (Lőrinc)
Pull request description:
Inspired by https://github.com/bitcoin/bitcoin/pull/32154, the main goal of this PR is to reenable sanitizer checks for `serialize.h` since it's modified in a few other PRs.
ACKs for top commit:
maflcko:
review ACK 516f0689b511c09153e4b6b4a58dfedd61c6cda7 🎈
stickies-v:
ACK 516f0689b511c09153e4b6b4a58dfedd61c6cda7, nice cleanup!
Tree-SHA512: 63da9bf1988a5b68e3c053b0ed786b8735f8f75b05763511522d1601b728b55798006e063137447615c266582622642d3226318fa83e488bd363f1756f8811e8
Would otherwise cause an OverflowError in feature_framework_startup_failures.py when calling subprocess.run() with 60 * factor.
Fixes#32506
Co-authored-by: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz>
Instead of failing during initialization when encountering a legacy wallet, skip
loading the wallet and notify the user accordingly.
This allows users to access migration functionalities without needing to manually
remove the wallet from settings.json or resort to using the bitcoin-wallet utility.
This means that GUI users will be able to use the migration button, and bitcoin-cli
users will be able to call the migratewallet RPC directly after init.
We would only modify the parent process' first --tmpdir arg.
Now we tack on an additional --tmpdir after the parent's arguments. Also simplifies the code.
Co-authored-by: Ryan Ofsky <ryan@ofsky.org>
75a185ea3db3177e8e479ee61a39bcb51e08d9a6 test: add skip_if_running_under_valgrind() (fanquake)
Pull request description:
Enable it in the USDT tests. The context (from 0xB10C):
> every time the tracepoint is reached a SIGTRAP is fired.
> No matter the tracepoint contents, even with an empty one.
> Valgrind intercepts SIGTRAP and aborts.
See discussion in #32374.
ACKs for top commit:
maflcko:
lgtm ACK 75a185ea3db3177e8e479ee61a39bcb51e08d9a6
willcl-ark:
ACK 75a185ea3db3177e8e479ee61a39bcb51e08d9a6
Tree-SHA512: 7f45c3049ab39cc514024067bd6ac26598e99202c114b48459834c26c2e1273fa58af693878298e628a10c561b954850e49e76b39567b771bb0c0534a063a524
faf9082a5f689e2e51a474bf654e4e9b6ca29685 test: Fix whitespace in prevector_tests.cpp (MarcoFalke)
fa7f04c8a7b7cbb4a1728bf2c9c6c7c8408b432a refactor: Remove UB in prevector reverse iterators (MarcoFalke)
Pull request description:
`rend()` creates a pointer with offset `-1`. This is UB, according to the C++ standard: https://eel.is/c++draft/expr.add#4:
When an expression J that has integral type is added to [...] an
expression P of pointer type, the result has the type of P.
... if P points to a (possibly-hypothetical) array element i of an
array object x with n elements [...] the expressions P + J and J + P
(where J has the value j) point to the (possibly-hypothetical) array
element i+j of x if 0≤i+j≤n [...]
Otherwise, the behavior is undefined.
Also, it is unclear why the functions exist at all, when stdlib utils such as `std::reverse_iterator{it}` or `std::views::reverse` can be used out of the box.
So remove them, along with the ubsan suppressions, that are no longer used.
I've tagged this a refactor, because the code was always dead (unused outside of tests). And since commit 2925bd537cbd8c70594e23f6c4298b7101f7f73d it was completely dead. Also, I could not find a sanitizer that detects this type of UB.
ACKs for top commit:
l0rinc:
tested ACK faf9082a5f689e2e51a474bf654e4e9b6ca29685
achow101:
ACK faf9082a5f689e2e51a474bf654e4e9b6ca29685
stickies-v:
ACK faf9082a5f689e2e51a474bf654e4e9b6ca29685, nice find.
theuni:
utACK faf9082a5f689e2e51a474bf654e4e9b6ca29685
Tree-SHA512: 31511d520a1c0fdd65c2e5f1a8ef6fd17464303b6bff88a5d9d9577adfee849d431deb510882b6f4e15e8fb7168861bc0d26fca3bed4278f57a9d6e7b1235dce
4b241867567203b204823a4558c2aa5767acf028 test: add test for decoding PSBT with MuSig2 PSBT key types (BIP 373) (Sebastian Falbesoner)
8ba245cb8318c9be375ac2fb2a3e7ded875b4408 test: add constants for MuSig2 PSBT key types (BIP 373) (Sebastian Falbesoner)
Pull request description:
This PR is a follow-up to #31247 (see https://github.com/bitcoin/bitcoin/pull/31247#pullrequestreview-2427834909) and adds a functional test for decoding PSBTs (using the `decodepsbt` RPC) with MuSig2 per-input and per-output types. The first commit adds the new MuSig2 key types to the test frameworks and extends the PSBT serialization to cope with lists of bytestrings.
ACKs for top commit:
achow101:
ACK 4b241867567203b204823a4558c2aa5767acf028
rkrux:
re-ACK 4b24186
Tree-SHA512: f12919f71b3fff74df1d7ddaa8db455b1b139f7abd51d7f3fa5d750fc7dd613454b438c4e0dedad679476d414fa1da43ef1121e486b0bdfd97d5ef8bdf37f060
Since CWallet::chainStateFlushed is now no-op, this test no longer tests
the concurrent writes scenario. There are no other cases where multiple
DatabaseBatches are open at the same time.
When a block is connected, if the new block had anything relevant to the
wallet, update the best block record on disk. If not, also sync the best
block record to disk every 144 blocks.
Also reuse the new WriteBestBlock method in BackupWallet.
Made every signed/unsigned conversion in the serialization helpers explicit so the UBSan `implicit-sign-change` check passes and the `serialize.h` suppression can be dropped.
For consistency, a few other simple changes were also applied to the serialization helpers:
* remove redundant `inline` on function templates;
* unify formatting to make the differences between similar methods obvious.
e7ad86e1ca3b0b2f2795e91c2f9959486c67dd90 test: fix another intermittent failure in wallet_basic.py (Martin Zumsande)
07350e204dedfba20da461d9cdcd469dc95e01c3 test: Fix intermittent failure in wallet_basic.py (Martin Zumsande)
Pull request description:
Fixes two rare failures that happened in the CI:
#27249:
There could be a race with outstanding TxAddedToMempool notifications being applied to the soon-to-be created wallet:
1. importdescriptors during rescan sets status to `TxStateConfirmed`
2. old `transactionAddedToMempool` notification changes status back to `TxStateInMempool`
3. If the listunspent call happens here the test will fail
4. blockConnected notification will change the status back to `TxStateConfirmed` (so it's not a persistent failure)
I could reproduce this by adding a 100 microsecond sleep to `AddToWallet()`, the fix is to add a sync, so `transactionAddedToMempool` notifications won't affect the new wallet anymore.
#32456:
During init, the test framework will start using rpc after the mempool was loaded.
It will not wait for `start()` / `postInitProcess` or outstanding `transactionAddedToMempool` notifications (which would both set the status to `TxStateInMempool`), leading to
a possible race, in which `listunspent` can be called while the tx is still in `Inactive` status.
Can be reproduced by adding two sleeps: To init before calling `start()` for the chain clients, plus to `transactionAddedToMempool` in `wallet.cpp`.
Prevent this by processing outstanding notifications.
Fixes#27249Fixes#32456
ACKs for top commit:
maflcko:
review ACK e7ad86e1ca3b0b2f2795e91c2f9959486c67dd90 🎩
Tree-SHA512: 1f1a11e5c8e1c6d3c39a49401c2c5122befdbbec25c0451953f5bfe8dfb53221ada552a68006e266570addda12bb16c1b9b1e49ad2198c33d91c4b96b764d73e
Enable it in the USDT tests. The context (from 0xB10C):
> every time the tracepoint is reached a SIGTRAP is fired.
> No matter the tracepoint contents, even with an empty one.
> Valgrind intercepts SIGTRAP and aborts.
See discussion in #32374.
rend() creates a pointer with offset -1. This is UB, according to the
C++ standard: https://eel.is/c++draft/expr.add#4:
When an expression J that has integral type is added to [...] an
expression P of pointer type, the result has the type of P.
... if P points to a (possibly-hypothetical) array element i of an
array object x with n elements [...] the expressions P + J and J + P
(where J has the value j) point to the (possibly-hypothetical) array
element i+j of x if 0≤i+j≤n [...]
Otherwise, the behavior is undefined.
Also, it is unclear why the functions exist at all, when stdlib utils
such as std::reverse_iterator{it} or std::views::reverse can be used out
of the box.
So remove them, along with the ubsan suppressions, that are no longer
used.
2aa63d511affdcc9980b58fc4ff18b8ad10b0f8c test: Use uninvolved pruned node in feature_pruning undo test (enoch)
772ba7f9ce09e836a51636524a8a96a23946d658 test: Fix nTimes typo in feature_pruning test (enoch)
Pull request description:
This PR contains two commits:
1. Fixes a typo in feature_pruning.py where 'nTimes' was incorrectly
used instead of 'nTime'. This typo caused the test to always reset
mine_large_blocks.nTime to 0, rather than only on the first run.
2. Fixes the test failure exposed by the typo fix. The
test_pruneheight_undo_presence test was failing because it was using
node 2, which is involved in reorg testing and could be on a
different chain than other nodes. The solution switches to using
node 5, which is also a pruned node but isn't involved in reorg
testing.
Testing:
- Ran test/functional/feature_pruning.py multiple times to verify
consistent passing
- Verified that the test now passes with the correct nTime variable name
- Confirmed the test behavior matches the intended functionality of
verifying pruned block availability
- Ran the full test suite to ensure the changes did not introduce any
regressions or affect other tests
Thanks to fjahr for his assistance in diagnosing the issue and
suggesting the solution.
This fixes the test failure reported in #32249
ACKs for top commit:
fjahr:
tACK 2aa63d511affdcc9980b58fc4ff18b8ad10b0f8c
maflcko:
lgtm ACK 2aa63d511affdcc9980b58fc4ff18b8ad10b0f8c
naiyoma:
tACK 2aa63d511affdcc9980b58fc4ff18b8ad10b0f8c
stratospher:
tested ACK 2aa63d5. verified that `nTime` is being incremented now.
Tree-SHA512: a543528fd4eeb30e978c0b43cfa109768252edaf1f94679dbbc7fe684122c00da34224e2cc1abd2a265af1b267eef1cd34246207946cf7d8e93d2c0f11aa56d8
During init, the test framework will start using rpc after the
mempool was loaded. It will not wait for postInitProcess or
outstanding transactionAddedToMempool notifications, leading to
a possible race, in which listunspent is being called while the
tx is still in Inactive status. Prevent this by processing
outstanding notifications.
There could be a race with outstanding TxAddedToMempool notifications
being applied to the soon-to-be created wallet.
Fixes an intermittent timeout reproducable by adding a sleep to
AddToWallet.
`-proxy=addr:port` specifies the proxy for all networks (except I2P).
Previously only the Tor proxy could have been specified separately via
`-onion=addr:port`.
Make it possible to specify separately the proxy for IPv4, IPv6, Tor and
CJDNS by e.g. `-proxy=addr:port=ipv6`. Or remove the proxy for a given
network, e.g. `-proxy=0=cjdns`.
Resolves: https://github.com/bitcoin/bitcoin/issues/24450
Support new BITCOIN_CMD environment variable in functional test to be able to
test the new bitcoin wrapper executable and run other commands through it
instead of calling them directly.
Co-authored-by: Sjors Provoost <sjors@sprovoost.nl>
a58cb3b1c12c8cb75a87375c50f94c4605bb805d qa: sanity check mined block have their coinbase timelocked to height (Antoine Poinsot)
8f2078af6a55448c003b3f7f3021955fbb351caa miner: timelock coinbase transactions (Antoine Poinsot)
788aeebf343526760fa8f3ed969ac3713212a5b6 qa: use prev height as nLockTime for coinbase txs created in unit tests (Antoine Poinsot)
c76dbe9b8b6f03b761a0ef97e1b8cd133b934714 qa: timelock coinbase transactions created in fuzz targets (Antoine Poinsot)
9c94069d8b6cf67a24eb03c51230a4f2b2bf2d64 contrib: timelock coinbase transactions in signet miner (Antoine Poinsot)
a5f52cfcc400ad0adb41a78c65b8abb971e0d622 qa: timelock coinbase transactions created in functional tests (Antoine Poinsot)
Pull request description:
The Consensus Cleanup soft fork proposal includes enforcing that coinbase transactions set their
nLockTime field to the block height minus 1, as well as their nSequence such as to not disable the
timelock. If such a fork were to be activated by Bitcoin users, miners need to be ready to produce
compliant blocks at the risk of losing substantial amounts mining would-be invalid blocks. As miners
are unfamously slow to upgrade, it's good to make this change as early as possible.
Although Bitcoin Core's GBT implementation does not provide the `coinbasetxn` field, and mining
pool software crafts the coinbase on its own, updating the Bitcoin Core mining code is a first step
toward convincing pools to update their (often closed source) code. A possible followup is also to
introduce new fields to GBT. In addition, this first step also makes it possible to test future
Consensus Cleanup changes.
The commit making the change also updates a bunch of seemingly-unrelated tests. This is because those tests were asserting error messages based on the txid of transactions involved, and changing the coinbase transaction structure necessarily changes the txid of all tests' transactions.
ACKs for top commit:
Sjors:
Code review ACK a58cb3b1c12c8cb75a87375c50f94c4605bb805d
achow101:
ACK a58cb3b1c12c8cb75a87375c50f94c4605bb805d
TheCharlatan:
Re-ACK a58cb3b1c12c8cb75a87375c50f94c4605bb805d
Tree-SHA512: a2aae009a187eb760d34435f518a895ee76c6b02a667eb030ddf6bd584da6e8eae2737d974dbf81a928d60c07bcb4820f055adc067e18d8819640db0240bb513
