It is UB to apply a distance to a pointer or iterator further than the
end itself, even if the distance is (partially) revoked later on.
Fix the issue by advancing the data pointer at most to the end.
In almost all cases (the only exception is `getifaddrs`), we know the
size of the data passed into SetSockAddr, so we can check this to be
what is expected.
69e95c2b4f99eb4c2af6b2b6cc6a66abfea753df tests: Test cleanup of mkeys from wallets without privkeys (Andrew Chow)
2b9279b50a3682ab97308888b4f272d3ae379811 wallet: Remove unused encryption keys from watchonly wallets (Andrew Chow)
813a16a463326079472366aed42f5218a57db63b wallet: Add HasCryptedKeys (Andrew Chow)
Pull request description:
An earlier version allowed users to create watchonly wallets (wallets without private keys) that were "encrypted". Such wallets would have a stored encryption keys, but nothing would actually be encrypted with them. This can cause unexpected behavior such as https://github.com/bitcoin-core/gui/issues/772.
We can detect such wallets as they will have the disable private keys flag set, no encrypted keys, and encryption keys. For such wallets, we can remove those encryption keys thereby avoiding any issues that may result from this unexpected situation.
ACKs for top commit:
sipa:
utACK 69e95c2b4f99eb4c2af6b2b6cc6a66abfea753df.
laanwj:
Code review re-ACK 69e95c2b4f99eb4c2af6b2b6cc6a66abfea753df
furszy:
Code review ACK 69e95c2b4f99eb4
Tree-SHA512: 901932cd709c57e66c598f011f0105a243b5a8b539db2ef3fcf370dca4cf35ae09bc1110e8fca8353be470f159468855a4dd96b99bc9c1112adc86ccc50e1b9d
e04be3731f4921cd51d25b1d6210eace7600fea4 init,log: Unify block index and chainstate loading log line (Lőrinc)
Pull request description:
The line has been present since the beginning.
Removed redundant duration as well since it can be recovered from the timestamps.
Example logs before the change:
```
2025-01-07T11:58:33Z Verification progress: 99%
2025-01-07T11:58:33Z Verification: No coin database inconsistencies in last 6 blocks (18905 transactions)
2025-01-07T11:58:33Z block index 31892ms
2025-01-07T11:58:33Z Setting NODE_NETWORK on non-prune mode
```
ACKs for top commit:
maflcko:
lgtm ACK e04be3731f4921cd51d25b1d6210eace7600fea4
TheCharlatan:
ACK e04be3731f4921cd51d25b1d6210eace7600fea4
danielabrozzoni:
tACK e04be3731f4921cd51d25b1d6210eace7600fea4
BrandonOdiwuor:
Code Review ACK e04be3731f4921cd51d25b1d6210eace7600fea4
Tree-SHA512: cbe4569a17f56ff23e829b837a083c2f730cc490b47bee3bac12126e2257e0ba9ebe9b4384deb03203a0a60aac3b8d283c5d31a6d0481635ba011ac6e2c61ad1
a96b84cb1b76e65a639e62f0224f534f89858c18 fuzz: Abort when calling system time without setting mock time (marcofleon)
ff21870e20b2391b684cc50fdd6879805055d6a1 fuzz: Add SetMockTime() to necessary targets (marcofleon)
Pull request description:
This PR expands the `CheckGlobals` utility that was introduced in https://github.com/bitcoin/bitcoin/pull/31486 and should help with fuzz stability (https://github.com/bitcoin/bitcoin/issues/29018).
System time shouldn't be used when running a fuzz test, as it is likely to introduce instability (non-determinism). This PR identifies and fixes the targets that were calling system time without setting mock time at the start of an iteration.
Removing`SetMockTime()` from any one of these targets should result in a crash and a message describing the issue.
ACKs for top commit:
achow101:
ACK a96b84cb1b76e65a639e62f0224f534f89858c18
dergoegge:
Code review ACK a96b84cb1b76e65a639e62f0224f534f89858c18
brunoerg:
crACK a96b84cb1b76e65a639e62f0224f534f89858c18
Tree-SHA512: e093a9feb8a397954f7b1416dfa8790b2733f09d5ac51fda5a9d225a55ebd8f99135aa52bdf5ab531653ad1a3739c4ca2b5349c1d989bb4b009ec8eaad684f7d
fd2d96d9087be234bdf4a6eb6d563e92b4fb4501 build, test: Build `db_tests.cpp` regardless of `USE_BDB` (Hennadii Stepanov)
Pull request description:
When the building of `db_tests.cpp` was made conditional on `USE_BDB` in commit a58b719cf75e2d97205ec260bcff0d4780fe4fb8, all `db_tests` were indeed specific to BDB wallets.
However, the tests have since been [extended](ba616b932c) to include SQLite wallets as well.
On the master branch @ 433412fd8478923dfdb20044f74c5d1e19fa8dd8, tests specific to SQLite wallets are not built and run if configured with `WITH_BDB=OFF` (the default option).
This PR resolves this issue by guarding BDB-specific code in `db_tests.cpp` and ensuring this source file is compiled regardless of the `WITH_BDB` option.
ACKs for top commit:
achow101:
ACK fd2d96d9087be234bdf4a6eb6d563e92b4fb4501
maflcko:
review ACK fd2d96d9087be234bdf4a6eb6d563e92b4fb4501 🔺
theuni:
utACK fd2d96d9087be234bdf4a6eb6d563e92b4fb4501
Tree-SHA512: bd9eddf16af60c568e931467d39e9e23a268e82e367ab630c23ac3cfd37e6007c6d78579b69ccbeebc1911c749cdbe75794fd56d7fbdb30c6fea6d2ab11017a3
589ed1a8eafe1daed379ebb383602c8f220aef19 wallet: migration, avoid loading wallet after failure when it wasn't loaded before (furszy)
Pull request description:
Fixes#31447.
During migration failure, only load wallet back into memory when the wallet was
loaded prior to migration. This fixes the case where BDB is not supported, which
implies that no legacy wallet can be loaded into memory due to the lack of db
writing functionality.
Link to error description https://github.com/bitcoin/bitcoin/issues/31447#issuecomment-2528757140.
This PR also improves migration backup related comments to better document the
current workflow.
ACKs for top commit:
achow101:
ACK 589ed1a8eafe1daed379ebb383602c8f220aef19
rkrux:
ACK 589ed1a8eafe1daed379ebb383602c8f220aef19
pablomartin4btc:
tACK 589ed1a8eafe1daed379ebb383602c8f220aef19
Tree-SHA512: c7a489d2b253c574ee0287b691ebe29fe8d026f659f68a3f6108eca8b4e1e420c67ca7803c6bd70c1e1440791833fabca3afbcf8fe8524c6c9fc08de95b618d0
If the chain advances during a rescan, ScanForWalletTransactions
would previously process the new blocks without adjusting m_last_processed_block,
which would leave the wallet in an inconsistent state temporarily, and could lead
to crashes in the GUI.
Fix this by not rescanning blocks beyond the last_processed_block -
for all blocks beyond that height, there will be pending BlockConnected
notifications that will process them after the rescan is finished.
Co-authored-by: Pablo Greco <psgreco@gmail.com>
When the behavior was changes in a previous commit (caching `GetSerializeSize` and avoiding `AutoFile.tell`), (static)asserts were added to make sure the behavior was kept - to make sure reviewers and CI validates it.
We can safely remove them now.
Logs were also slightly modernized since they were trivial to do.
Co-authored-by: Anthony Towns <aj@erisian.com.au>
Co-authored-by: Hodlinator <172445034+hodlinator@users.noreply.github.com>
For consistency `UNDO_DATA_DISK_OVERHEAD` was also extracted to avoid the constant's ambiguity.
Asserts were added to help with the review - they are removed in the next commit.
Co-authored-by: Ryan Ofsky <ryan@ofsky.org>
Example logs before the change:
```
2025-01-07T11:58:33Z Verification progress: 99%
2025-01-07T11:58:33Z Verification: No coin database inconsistencies in last 6 blocks (18905 transactions)
2025-01-07T11:58:33Z block index 31892ms
2025-01-07T11:58:33Z Setting NODE_NETWORK on non-prune mode
2025-01-07T11:58:33Z block tree size = 878086
2025-01-07T11:58:33Z nBestHeight = 878085
```
Removed redundant duration as well since it can be recovered from the timestamps.
Co-authored-by: TheCharlatan <seb.kung@gmail.com>
Co-authored-by: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz>
Similarly, `WriteBlockToDisk` wasn't really extracting a meaningful subset of the `SaveBlockToDisk` functionality, it's tied closely to the only caller (needs the header size twice, recalculated block serializes size, returns multiple branches, mutates parameter).
The inlined code should only differ in these parts (modernization will be done in other commits):
* renamed `blockPos` to `pos` in `SaveBlockToDisk` to match the parameter name;
* changed `return false` to `return FlatFilePos()`.
Also removed remaining references to `SaveBlockToDisk`.
Co-authored-by: Ryan Ofsky <ryan@ofsky.org>
`UndoWriteToDisk` wasn't really extracting a meaningful subset of the `WriteUndoDataForBlock` functionality, it's tied closely to the only caller (needs the header size twice, recalculated undo serializes size, returns multiple branches, modifies parameter, needs documentation).
The inlined code should only differ in these parts (modernization will be done in other commits):
* renamed `_pos` to `pos` in `WriteUndoDataForBlock` to match the parameter name;
* inlined `hashBlock` parameter usage into `hasher << block.pprev->GetBlockHash()`;
* changed `return false` to `return FatalError`;
* capitalize comment.
Co-authored-by: Ryan Ofsky <ryan@ofsky.org>
Done in separate commit to simplify review.
Also renames benchmarks, since they're not strictly tests.
Co-authored-by: Hodlinator <172445034+hodlinator@users.noreply.github.com>
(total_cache / 4) + (1 << 23) is at least 8 MiB and nMaxCoinsDBCache is
also 8 MiB, so the minimum between the two will always be
nMaxCoinsDBCache. This is just a simplification and not changing the
result of the calculation.
Co-authored-by: Ryan Ofsky <ryan@ofsky.org>
- The package feerates are ordered by the sequence in which
packages are selected for inclusion in the block template.
- The commit also tests this new behaviour.
Co-authored-by: willcl-ark <will@256k1.dev>
3e0a992a3f0f2b15b7be5049dc4f3134b4b0bc40 doc: Clarify comments about endianness after #30526 (Ryan Ofsky)
Pull request description:
This is a documentation-only change following up on suggestions made in the #30526 review.
Motivation for this change is that I was recently reviewing #31583, which reminded me how confusing the arithmetic blob code was and made me want to write better comments.
ACKs for top commit:
achow101:
ACK 3e0a992a3f0f2b15b7be5049dc4f3134b4b0bc40
TheCharlatan:
ACK 3e0a992a3f0f2b15b7be5049dc4f3134b4b0bc40
Sjors:
ACK 3e0a992a3f0f2b15b7be5049dc4f3134b4b0bc40
BrandonOdiwuor:
LGTM ACK 3e0a992a3f0f2b15b7be5049dc4f3134b4b0bc40
Tree-SHA512: 90d5582a25a51fc406d83ca6b8c4e5e4d3aee828a0497f4b226b2024ff89e29b9b50d0ae8ddeac6abf2757fe78548d58cf3dd54df4b6d623f634a2106048091d
04249682e381f976de6ba56bb4fb2996dfa194ab test: use Mining interface in miner_tests (Sjors Provoost)
Pull request description:
Needed for both #31283 and #31564.
By using the Mining interface in `miner_tests.cpp` we increase its coverage in unit tests.
ACKs for top commit:
achow101:
ACK 04249682e381f976de6ba56bb4fb2996dfa194ab
ryanofsky:
Code review ACK 04249682e381f976de6ba56bb4fb2996dfa194ab, just minor suggested changes (renames, comments, BOOST_REQUIREs) since last review and some more extra clarifications and checks added to the CreateNewBlock_validity test. The CreateNewBlock_validity changes seem clear and easy to understand now.
vasild:
ACK 04249682e381f976de6ba56bb4fb2996dfa194ab
tdb3:
ACK 04249682e381f976de6ba56bb4fb2996dfa194ab
Tree-SHA512: 2761cb7555d759670e40d8f37b96a079f8e12a588ac43313b9e63c69afd478321515873a8896ea56784f0100dac4476b0c0e0ef8b5418f8aea24d9965cace4d4
SIGHASH_DEFAULT should be used to indicate SIGHASH_DEFAULT for taproot
inputs, and SIGHASH_ALL for all other input types. This avoids adding an
unnecessary byte to the end of all Taproot signatures added to PSBTs
signed in the GUI.
GetPrivKey() needs the same handling of all keyids for xonly keys that
ToPrivateString() does. Refactor that into GetPrivKey() and reuse it in
ToPrivateString() to resolve this.
fa397177acfa1006ea6feee0b215c53e51f284de util: Add missing types in make_secure_unique (MarcoFalke)
Pull request description:
The return type of `std::forward` depends on the template type, and can not be recovered from the args. Attempting to do so will result in a compile failure. For example, `make_secure_unique<std::string>(std::string{});` does not compile on current master, but does with this pull.
Another example would be `make_secure_unique<std::pair<std::string, std::unique_ptr<int>>>(std::string{}, std::make_unique<int>(21));`
ACKs for top commit:
hodlinator:
ACK fa397177acfa1006ea6feee0b215c53e51f284de
hebasto:
ACK fa397177acfa1006ea6feee0b215c53e51f284de.
TheCharlatan:
ACK fa397177acfa1006ea6feee0b215c53e51f284de
Tree-SHA512: cc902c1111c929a79a6f806b5097136a465e8c727474176bad30a5777ebbb30bedb0bd35273b43bf839d2c00492500ddec724bd17349250451f6b329cb71e6f2
Now that we track all announcers of an orphan, it's not helpful to
consider an orphan provided by a peer that didn't send us this parent.
It can only hurt our chances of finding the right orphan when there are
multiple candidates.
Adapt the 2 tests in p2p_opportunistic_1p1c.py that looked at 1p1c
packages from different peers. Instead of checking that the right peer
is punished, we now check that the package is not submitted. We can't
use the functional test to see that the package was not considered
because the behavior is indistinguishable (except for the logs).
This means we no longer return parents we already have in the
m_unique_parents result from MempoolRejectedTx.
We need to separate the loop that checks AlreadyHave parents from the
loop that adds parents as announcements, because we may do the latter
loop multiple times for different peers.
Add ability to add and track multiple announcers per orphan transaction,
erasing announcers but not the entire orphan.
The tx creation code in orphanage_tests needs to be updated so that each
tx is unique, because the CountOrphans() check assumes that calling
EraseForPeer necessarily means its orphans are deleted.
Unused for now.
