7193245cd66791216d4e586ca09302b26d4b7528 doc: remove For ... comments (fanquake)
1b9cdc933f6c11cb8593b5ad9ae3f4eb2c726859 net: drop win32 ifdef (fanquake)
19ba499b1f3884ea69c5f833f3eb797f90372aa5 init: cerrno is used on all platforms (fanquake)
Pull request description:
We don't add or maintain these, and they are of little value, as
well as having the effect of polluting diffs, if changed.
They are also wrong, i.e `DEFAULT_SCRIPTCHECK_THREADS` is not in
`validation.h`.
ACKs for top commit:
stickies-v:
re-ACK 7193245cd66791216d4e586ca09302b26d4b7528
fjahr:
ACK 7193245cd66791216d4e586ca09302b26d4b7528
willcl-ark:
reACK 7193245cd66791216d4e586ca09302b26d4b7528
Tree-SHA512: 6b5f83cd1df699356e1cbb78949f8d456b13ce288f0064138118cfb45b4c77e2d1945babe91598dffe9823ab07dfae36f4c3b61c586cf98baf16890bdf322b08
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
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
fa982f14254433a969499e93c1c3f0db31dce6ab Use WitnessV0KeyHash in TestAddAddressesToSendBook (MarcoFalke)
Pull request description:
Fixes https://github.com/bitcoin/bitcoin/issues/32558
Fixes https://github.com/bitcoin-core/gui/issues/874
This fixes a bug introduced in commit fafee85358397289aa4c6b799d2603a5d89e83a2, which changed the type of the dummy address from `WitnessV0KeyHash` to `PKHash`. It was expected that this is fine, given that this is just a dummy address. However, the base58 characters can include the substring "io", leading to test failures later on.
Fix it by just using `WitnessV0KeyHash` again.
For reference, a passing test could look like:
```
Model contains 2 rows and 2 columns.
--- Model Data ---
Row 0 : "io - new A\tmxgkqJWAwfUwbgzZUsWrG1stKWV6fDn8YH"
Row 1 : "io - new B\tmhsxP2yrYDQiEncT8HzKxQSFSFJmUsudsP"
------------------
```
A failing test could look like:
```
Model contains 3 rows and 2 columns.
--- Model Data ---
Row 0 : "already here (s)\tmyDFZSKDQdPMMoSQgzkDtq2yioo8DA8qCX"
Row 1 : "io - new A\tmsAqQKjMrbxYRDhGXBBJ3yUEQxj5Bf5Njz"
Row 2 : "io - new B\tmtALQiit8dw33kznVfHDgE38ohfgz2Pchc"
------------------
FAIL! : AddressBookTests::addressBookTests() Compared values are not the same
Actual (table_view->model()->rowCount()): 3
Expected (2) : 2
Loc: [qt/test/addressbooktests.cpp(219)]
```
ACKs for top commit:
achow101:
ACK fa982f14254433a969499e93c1c3f0db31dce6ab
hebasto:
ACK fa982f14254433a969499e93c1c3f0db31dce6ab, I have reviewed the code along with the related changes from https://github.com/bitcoin/bitcoin/pull/32511.
Tree-SHA512: f55d7fe4193a0706e1a3ca1a2c0fbf2f04dc5b177699add00013ec56d64218ac85b80dad6e99f9fde26f4c9fca79f99e68ded057c5862364064404ac06b77176
e49a7274a2141dcb9e188bc4b45c2d7b928ccecd rpc: Avoid join-split roundtrip for user:pass for auth credentials (Vasil Dimov)
98ff38a6f1a8a1e214bd3905a2dcac31ae6c2f52 rpc: Perform HTTP user:pass split once in `RPCAuthorized` (laanwj)
879a17bcb1a5eab2ff1841ce5f3762dcccecb0ba rpc: Store all credentials hashed in memory (laanwj)
4ab9bedee9d86fdecaa4afbbf4214ca6c7d9a94e rpc: Undeprecate rpcuser/rpcpassword, change message to security warning (laanwj)
Pull request description:
This PR does two things:
### Undeprecate rpcuser/rpcpassword, change message to security warning
Back in 2015, in https://github.com/bitcoin/bitcoin/pull/7044, we added configuration option `rpcauth` for multiple RPC users. At the same time the old settings for single-user configuration `rpcuser` and `rpcpassword` were "soon" to be deprecated.
The main reason for this deprecation is that while `rpcpassword` stores the password in plain text, `rpcauth` stores a hash, so it doesn't appear in the configuration in plain text.
As the options are still in active use, actually removing them is expected to be a hassle to many, and it's not clear that is worth it. As for the security risk, in many kinds of setups (no wallet, containerized, single-user-single-application, local-only, etc) it is an unlikely point of escalation.
In the end, it is good to encourage secure practices, but it is the responsibility of the user. Log a clear warning but remove the deprecation notice (this is also the only place where the options appear as deprecated, they were never marked as such in the -help output).
<hr>
### Store all credentials hashed in memory
This gets rid of the special-casing of `strRPCUserColonPass` by hashing cookies as well as manually provided `-rpcuser`/`-rpcpassword` with a random salt before storing them.
Also take the opportunity to modernize the surrounding code a bit. There should be no end-user visible differences in behavior.
<hr>
Closes#29240.
ACKs for top commit:
1440000bytes:
utACK e49a7274a2
janb84:
reACK e49a7274a2
vasild:
ACK e49a7274a2141dcb9e188bc4b45c2d7b928ccecd
Tree-SHA512: 7162848ada4545bc07b5843d1ab6fb7e31fb26de8d6385464b7c166491cd122eac2ec5e70887c414fc136600482df8277dc0cc0541d7b7cf62c4f72e25bb6145
We don't add or maintain these, and they are of little value, as
well as having the effect of polluting diffs.
They are also wrong, i.e DEFAULT_SCRIPTCHECK_THREADS is not in
validation.h.
fa1f10a49e7c4f6377fbc7ae2f1520b38c86e5fa doc: Fix minor typos in rpc help (MarcoFalke)
fae840e94b79c427215c13e0fd5a9fcd71295ec9 rpc: Reject beginning newline in RPC docs (MarcoFalke)
fa414eda0834f6e8260855a01122b1bc1100922f scripted-diff: Remove unused leading newline in RPC docs (MarcoFalke)
Pull request description:
It is harmless, but newlines in the beginning read a bit odd ("nReturns"). So just require them to not be present.
The diff is large, but a trivial scripted-diff.
ACKs for top commit:
fanquake:
ACK fa1f10a49e7c4f6377fbc7ae2f1520b38c86e5fa
w0xlt:
ACK fa1f10a49e
Tree-SHA512: 5d2f9632f42ec1c02814d050f223941f436e5b0df426d7d6eb93fdd0ff118d57185af07b271dd73af63735dd17231125826c0c9ce0aad36bc8829c5b050a628c
7015052eba23368539dcd1a9b4217ce1cacd2999 build: remove Wsuggest-override suppression from leveldb build (fanquake)
e2c84b896fad37cbbf40c02f24c058b641e5bccc Squashed 'src/leveldb/' changes from 4188247086..113db4962b (fanquake)
Pull request description:
Pulls in
* https://github.com/bitcoin-core/leveldb-subtree/pull/51
Remove the related warning suppression.
ACKs for top commit:
l0rinc:
utACK 7015052eba23368539dcd1a9b4217ce1cacd2999
hebasto:
ACK 7015052eba23368539dcd1a9b4217ce1cacd2999, I've updated the `leveldb` subtree locally and got zero diff with this branch.
Tree-SHA512: 1ac7c8ecc9025086b429e12c22fc25f654eaf68fc9500b95341fb635cea12e7f80d69298cff120e8557a4f2f5809956a3b158cdb4db745cfa605c0df6f346423
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
301993ebf7f8ec23050e91377e0fd05823bb372a init: drop -upnp (fanquake)
Pull request description:
This was slated for removal in `30.0`, so remove it.
ACKs for top commit:
i-am-yuvi:
ACK 301993ebf7f8ec23050e91377e0fd05823bb372a
maflcko:
review ACK 301993ebf7f8ec23050e91377e0fd05823bb372a
darosior:
tACK 301993ebf7f8ec23050e91377e0fd05823bb372a
Tree-SHA512: 635e374c013fa08c4cda7caadc465c89bb376d3ee2c66f67a27e3ed9031844674d3e996169aaffb9b65a67b0d44d92aaec000aaf69abe3dd10fce2f4138f3e27
8f4fed7ec70093e2535423d63e9f9dd400c378ac symbol-check: Add check for application manifest in Windows binaries (Hennadii Stepanov)
2bb6ab8f1baa4b3d72b3ccde7f5fa96f8ca330aa ci: Add "Get bitcoind manifest" steps to Windows CI jobs (Hennadii Stepanov)
282b4913c7e4d4b5a141c9f89da97a65ee86bdd9 cmake: Add application manifests when cross-compiling for Windows (Hennadii Stepanov)
Pull request description:
Windows [application manifests ](https://learn.microsoft.com/en-us/windows/win32/sbscs/application-manifests) provide several benefits—such as enhanced security settings, and the ability to set a process-wide code page (required for https://github.com/bitcoin/bitcoin/pull/32380), as well as granular control over supported Windows versions. Most of these benefits lie beyond the scope of this PR and will be evaluated separately.
On the current master branch @ fc6346dbc8dc3db40aad4079210332b5f8b332ed, the linker generates and embeds a manifest only when building with MSVC:
```xml
<?xml version="1.0" encoding="UTF-8" standalone="yes"?>
<assembly xmlns="urn:schemas-microsoft-com:asm.v1" manifestVersion="1.0">
<trustInfo xmlns="urn:schemas-microsoft-com:asm.v3">
<security>
<requestedPrivileges>
<requestedExecutionLevel level="asInvoker" uiAccess="false"></requestedExecutionLevel>
</requestedPrivileges>
</security>
</trustInfo>
</assembly>
```
However, this manifest fails validation:
```
> mt.exe -nologo -inputresource:build\bin\Release\bitcoind.exe -validate_manifest
mt.exe : general error 10100ba: The manifest is missing the definition identity.
```
This PR unifies manifest embedding for both native and cross-compilation builds.
Here is the change in the manifest on Windows:
```diff
--- bitcoind-master.manifest
+++ bitcoind-pr.manifest
@@ -1,5 +1,6 @@
<?xml version="1.0" encoding="UTF-8" standalone="yes"?>
<assembly xmlns="urn:schemas-microsoft-com:asm.v1" manifestVersion="1.0">
+ <assemblyIdentity type="win32" name="org.bitcoincore.bitcoind" version="29.99.0.0"></assemblyIdentity>
<trustInfo xmlns="urn:schemas-microsoft-com:asm.v3">
<security>
<requestedPrivileges>
```
which effectively resolves the "missing the definition identity" error.
Finally, “Get bitcoind manifest” steps have been added to the Windows CI jobs to ensure the manifest is embedded and validated.
ACKs for top commit:
sipsorcery:
re-tACK 8f4fed7ec70093e2535423d63e9f9dd400c378ac.
hodlinator:
re-ACK 8f4fed7ec70093e2535423d63e9f9dd400c378ac
davidgumberg:
Reviewed and tested ACK 8f4fed7ec7
Tree-SHA512: 6e2dbdc77083eafdc242410eb89a6678e37b11efd786505dcd7844f0bac8f44d68625e62924a03b26549bdb4aaec5330dc608e6b4d66789f0255092e23aef6cb
0671d66a8ee07584fab6f1ddaaa188c6a4ac25c1 wallet, refactor: Convert uint256 to Txid in wallet (marcofleon)
c8ed51e62be998f16b8b06201b1df92b73c4220d wallet, refactor: Convert uint256 to Txid in wallet interfaces (marcofleon)
b3214cefe6d880838f36e7801bc2c3068bd98d96 qt, refactor: Convert uint256 to Txid in the GUI (marcofleon)
Pull request description:
This is part of https://github.com/bitcoin/bitcoin/pull/32189.
Converts all instances of transactions from `uint256` to `Txid` in the wallet, GUI, and related interfaces.
ACKs for top commit:
stickies-v:
re-ACK 0671d66a8ee07584fab6f1ddaaa188c6a4ac25c1, no changes since 65fcfbb2b38bef20a58daa6c828c51890180611d except rebase.
achow101:
ACK 0671d66a8ee07584fab6f1ddaaa188c6a4ac25c1
furszy:
Code review ACK 0671d66a8ee07584fab6f1ddaaa188c6a4ac25c1
Tree-SHA512: 9fd4675db63195c4eed2d14c25015a1821fb597f51404674e4879a44a9cf18f475021a97c5f62f3926b7783ade5a38567386f663acba9f5861f1f59c1309ed60
This reverts commit b2d536100282bd901d3e0be7f7f4a6966e0ef817.
Also, adjust the doc to reflect the new minimum version. Versions 17.6
or 17.11 (or anything in between) may still work on a best-effor basis,
but it is not checked by CI or by developers.
It is only used in test. There it is problematic, because it sometimes
relies on m_default_address_type. If the default were changed to
BECH32M, those tests would fail the assert(false).
So just use PKHash{} in all tests and remove GetDestinationForKey.
Windows application manifests provide several benefits. However, on the
master branch, the linker generates and embeds manifests only when
building with MSVC.
This change unifies manifest embedding for both native and
cross-compilation.
a0eed55398f882d9390e50582b10272d18f2b836 run_command: Enable close_fds option to avoid lingering fds (Luke Dashjr)
c7c356a448657e154e6bad6c39a296cfd6dce30c cpp-subprocess: Iterate through /proc/self/fd for close_fds option on Linux (Luke Dashjr)
4f5e04da135080291853f71e6f81dd0302224c3a Revert "remove unneeded close_fds option from cpp-subprocess" (Luke Dashjr)
Pull request description:
Picks up stale #30756, while addressing my fallback comment (https://github.com/bitcoin/bitcoin/pull/30756#discussion_r2030844440).
> Currently, RunCommandParseJSON runs its target with whatever fds happen to be open inherited on POSIX platforms. I don't think there's any practical scenario where this is a problem right now, but there's a lot of potential for weird problems (eg, if a process manages to outlive bitcoind - perhaps it's hanging - the listening port(s) won't get released and starting bitcoind again will fail). It's also a potential security issue if a child process is intended to be sandboxed at some point. Not to mention plain ugly :)
>
> cpp-subprocess has a feature to address this called close_fds. Not sure why it was removed in https://github.com/bitcoin/bitcoin/pull/29961 rather than fixing this during the migration, but this PR restores it, enables it for RunCommandParseJSON, and optimises it by iterating over /proc/self/fd/ like most other libraries do these days ([eg, glib]> (487b1fd20c/glib/gspawn.c (L1094))) since iterating all possible fd numbers [has been found to be problematic](https://bugzilla.redhat.com/show_bug.cgi?id=1537564).
>
> (Equivalent to https://github.com/bitcoin/bitcoin/pull/22417 was for boost::process)
ACKs for top commit:
achow101:
ACK a0eed55398f882d9390e50582b10272d18f2b836
hebasto:
ACK a0eed55398f882d9390e50582b10272d18f2b836, tested on Ubuntu 25.04:
vasild:
ACK a0eed55398f882d9390e50582b10272d18f2b836
Tree-SHA512: 7dc1cb6cc1f45ff7c4f53512e400baad1a033b4ebf14ba6f6ffa38588314932d6d01ef67b197f081e8202bb802659ac6a87998277797721d6d7b20efde8e9a6b
5bf91ba8800d23402536d758f02198eac0fd7d61 wallet: Drop unused fFromMe from CWalletTx (David Gumberg)
Pull request description:
This has been unused since commit fe52346, this is a re-opening of #9351.
ACKs for top commit:
maflcko:
lgtm ACK 5bf91ba8800d23402536d758f02198eac0fd7d61
achow101:
ACK 5bf91ba8800d23402536d758f02198eac0fd7d61
Tree-SHA512: b9a84f27b6cfe7796dcf629be6a8e01a97d931ea81ef088951d54d6691ffe79d22138baacc632375093cf3176a22c265e30a80f1f63c3bc620d08bf16f6a488f
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
62fc42d475df4f23bd93313f95ee7b7eb0d4683f interfaces: refactor: move `waitTipChanged` implementation to miner (ismaelsadeeq)
c39ca9d4f7bc9ca155692ac949be2e61c0598a97 interfaces: move getTip implementation to miner (Sjors Provoost)
720f201e652885b9e0aec8e62a1bf9590052b320 interfaces: refactor: move `waitNext` implementation to miner (ismaelsadeeq)
e6c2f4ce7a841153510971f0236c527d1a499649 interfaces: refactor: move `submitSolution` implementation to miner (ismaelsadeeq)
02d4bc776bbe002ee624ec2c09d7c3f981be1b17 interfaces: remove redundant coinbase fee check in `waitNext` (ismaelsadeeq)
Pull request description:
#### Motivation
In [Internal interface guidelines](https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#internal-interface-guidelines)
It's stated that
> Interface method definitions should wrap existing functionality instead of implementing new functionality. Any substantial new node or wallet functionality should be implemented in [src/node/](https://github.com/bitcoin/bitcoin/blob/master/src/node) or [src/wallet/](https://github.com/bitcoin/bitcoin/blob/master/src/wallet) and just exposed in [src/interfaces/](https://github.com/bitcoin/bitcoin/blob/master/src/interfaces) instead of being implemented there, so it can be more modular and accessible to unit tests.
However the some methods in the newly added `BlockTemplateImpl` and `MinerImpl` classes partially enforces this guideline, as the implementations of the `submitSolution`, `waitNext`, and `waitTipChanged` methods reside within the class itself.
#### What the PR Does
This PR introduces a simple refactor by moving certain method implementations from `BlockTemplateImpl` into the miner module. It introduces three new functions:
1. Remove rundundant coinbase fee check in `waitNext`
2. **`AddMerkleRootAndCoinbase`**: Computes the block's Merkle root, inserts the coinbase transaction, and sets the Merkle root in the block. This function is called by `submitSolution` before the block is submitted for processing.
3. **`WaitAndCreateNewBlock`**: Returns a new block template either when transaction fees reach a certain threshold or when a new tip is detected. If a timeout is reached, it returns `nullptr`. The `waitNext` method in `BlockTemplateImpl` now simply wraps this function.
4. Move `GetTip` implementation to miner.
5. **`WaitTipChanged`**: Returns the tip when the chain it changes, or `nullopt` if a timeout or interrupt occurs. The `waitTipChanged` method in `MinerImpl` now calls `GetTip` after invoking `ChainTipChanged`, and returns the tip.
#### Behavior Change
- We now only `Assert` for a valid chainman and notifications pointer once.
ACKs for top commit:
achow101:
ACK 62fc42d475df4f23bd93313f95ee7b7eb0d4683f
Sjors:
ACK 62fc42d475df4f23bd93313f95ee7b7eb0d4683f
ryanofsky:
Code review ACK 62fc42d475df4f23bd93313f95ee7b7eb0d4683f. Lots of suggest suggest changes made since last review, altering function names and signatures and also adding new commit to drop negative fee handling. I like the idea of making the wait function return a BlockRef, that is clearer than what I suggested. Left some comments below but they are not important and this looks good as-is
Tree-SHA512: 502632f94ced81f576b2c43cf015f1527e2c259e6ca253f670f5a6889171e2246372b4e709575701afa3f01d488d6633557fef54f48fe83bbaf1836ac5326c4f
StopWallets, which was being called prior to UnloadWallets, performs an
unnecessary database closing step. This causes issues in UnloadWallets
which does additional database cleanups. Since the database closing step
is unnecessary, StopWallets is removed, and UnloadWallets is now called
from WalletLoaderImpl::stop.
chainStateFlushed is no longer needed since the best block is updated
after a block is scanned. Since the chainstate being flushed does not
necessarily coincide with the wallet having processed said block, it
does not entirely make sense for the wallet to be recording that block
as its best block, and this can cause race conditions where some blocks
are not processed. Thus, remove this notification.
Migrating a wallet requires having a bestblock record. This is always
the case in normal operation as such a record is always written on
wallet loading if it didn't already exist. However, within the unit
tests and benchmarks, this is not guaranteed. Since migration requires
the record, WalletMigration needs to also add this record before the
benchmark.
The only reason to call chainStateFlushed during wallet loading is to
ensure that the best block is written. Do these writes explicitly to
prepare for removing chainStateFlushed, while also ensuring that the
wallet's in memory state tracking is written to disk.
Additionally, after rescanning on wallet loading, instead of writing
the locator for the current chain tip, write the locator for the last
block that the rescan had scanned. This ensures that the stored best
block record matches the wallet's current state.
Any blocks dis/connected during the rescan are processed after the
rescan and the last block processed will be updated accordingly.
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.
a04f17a1882407db09b0a07338e12877ac1d9e92 doc: warn that CheckBlock() underestimates sigops (Sjors Provoost)
Pull request description:
Counting sigops in the witness requires context that `CheckBlock()` does not have, so it only counts sigops for non-segwit transactions.
It's useful to document, but it should not be a problem.
The commit message contains some historical context.
ACKs for top commit:
ismaelsadeeq:
ACK a04f17a1882407db09b0a07338e12877ac1d9e92
ryanofsky:
Code review ACK a04f17a1882407db09b0a07338e12877ac1d9e92
Tree-SHA512: 26528367a7f3cfa8540ef0b90f7aa912c8f0bc057428f20a1fd1d4e232dac77747bc20044f0fcb0ffab8a2e1fb3dbe3dab46be749553a917744ddc7a829025cb
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.
- This commit creates a function `WaitTipChanged` that waits for the connected
tip to change until timeout elapsed.
- This function is now used by `waitTipChanged`
Co-authored-by: Ryan Ofsky <ryan@ofsky.org>
fa427ffceeefd368a1ade273501ce4b01133ad4d fuzz: Properly setup wallet in wallet_fees target (MarcoFalke)
Pull request description:
`g_wallet_ptr` is destructed after the `testing_setup`. This is not supported and will lead to issues such as https://github.com/bitcoin/bitcoin/pull/30221#issuecomment-2863875857 or https://github.com/bitcoin/bitcoin/pull/32409#issuecomment-2855259932.
This could be fixed by fixing the initialization order.
However, the global wallet is also modified in the fuzz target, which is bad fuzzing practise.
So instead fix it by constructing a fresh wallet for each fuzz iteration.
ACKs for top commit:
brunoerg:
code review ACK fa427ffceeefd368a1ade273501ce4b01133ad4d
hebasto:
ACK fa427ffceeefd368a1ade273501ce4b01133ad4d, this change fixes the issue when building the "Debug" configuration with MSVC on Windows.
marcofleon:
Code review ACK fa427ffceeefd368a1ade273501ce4b01133ad4d
Tree-SHA512: 161b93fc39a609cb16d9ffea7366c5e339bd01712577f0782aedff46c00f79edd2a907807ac83f9fcec687b4bbbe0fd6e6f75e32169639a310e4e7b771078b3b
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.
