62fc42d475 interfaces: refactor: move `waitTipChanged` implementation to miner (ismaelsadeeq)
c39ca9d4f7 interfaces: move getTip implementation to miner (Sjors Provoost)
720f201e65 interfaces: refactor: move `waitNext` implementation to miner (ismaelsadeeq)
e6c2f4ce7a interfaces: refactor: move `submitSolution` implementation to miner (ismaelsadeeq)
02d4bc776b 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 62fc42d475
Sjors:
ACK 62fc42d475
ryanofsky:
Code review ACK 62fc42d475. 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.
a04f17a188 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 a04f17a188
ryanofsky:
Code review ACK a04f17a188
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>
fa427ffcee 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 fa427ffcee
hebasto:
ACK fa427ffcee, this change fixes the issue when building the "Debug" configuration with MSVC on Windows.
marcofleon:
Code review ACK fa427ffcee
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.
1e0de7a6ba fees: document non-monotonic estimation edge case (willcl-ark)
Pull request description:
Closes: https://github.com/bitcoin/bitcoin/issues/11800
In scenarios where data is available for higher targets but not for lower ones, this method *may* return lower fee rates for higher confirmation targets. This could occur if `estimateCombinedFee` returns no valid data (`-1`) for some estimates for a low target, but **does** return valid data for a higher target.
Users of this function should be aware of this potential, if unlikely, inconsistency in behaviour in data-sparse scenarios.
ACKs for top commit:
adamandrews1:
Code review ACK 1e0de7a
ismaelsadeeq:
Code review ACK 1e0de7a6ba
glozow:
ACK 1e0de7a6ba
Tree-SHA512: 161e5dafdd131570853a89491753ae39a7b725d1a86cab5a7294c2a5939da1a9a5f2c4aca0900e9ad810e828b6e0e636f256384e3d1fda6dd552da189bbbe747
0750249289 mining: document gbt_rule_value helper (Sjors Provoost)
5e87c3ec09 scripted-diff: rename gbt_force and gbt_force_name (Sjors Provoost)
Pull request description:
The term "force" is ambiguous and not used in [BIP9](https://github.com/bitcoin/bips/blob/master/bip-0009.mediawiki#getblocktemplate-changes) where there ! rule prefix is introduced.
E.g. this code is hard to read:
```cpp
if (!gbt_force) {
s.insert(s.begin(), '!');
```
Additionally, #29039 renamed `gbt_vb_name` to `gbt_force_name` which, at least for me, further increased the confusion.
This is a pure (variable rename) refactor (plus documentation) and does not change behavior.
Reminder of how to verify a scripted diff:
```sh
test/lint/commit-script-check.sh origin/master..HEAD
```
ACKs for top commit:
achow101:
ACK 0750249289
janb84:
ACK [0750249](0750249289)
musaHaruna:
ACK [0750249](0750249289)
glozow:
ACK 0750249289, seems sensible
Tree-SHA512: 8c88a273a3b36040f6c641843bd20579d0065b051aad4b39fc14f0d2af2808690dff6772bd8b1a4d9699b72279a700d2661012651bc315433a123dcc8996adaa
8673e8f019 txgraph: Special-case singletons in chunk index (optimization) (Pieter Wuille)
abdd9d35a3 txgraph: Skipping end of cluster has no impact (optimization) (Pieter Wuille)
604acc2c28 txgraph: Reuse discarded chunkindex entries (optimization) (Pieter Wuille)
c734081454 txgraph: Introduce TxGraph::GetWorstMainChunk (feature) (Pieter Wuille)
394dbe2142 txgraph: Introduce BlockBuilder interface (feature) (Pieter Wuille)
883df3648e txgraph: Generalize GetClusterRefs to support subsections (preparation) (Pieter Wuille)
c28a602e00 txgraph: Introduce TxGraphImpl observer tracking (preparation) (Pieter Wuille)
9095d8ac1c txgraph: Maintain chunk index (preparation) (Pieter Wuille)
87e74e1242 txgraph: abstract out transaction ordering (refactor) (Pieter Wuille)
2614fea17f txgraph: Add GetMainStagingDiagrams function (feature) (Pieter Wuille)
Pull request description:
Part of cluster mempool: #30289.
This adds more functionality to the txgraph module, specifically:
* `TxGraph::GetMainStagingDiagrams()`, a function to obtain feerate diagrams for both the main graph and the staged changes to it, including only the clusters that differ between the two.
* `TxGraph::GetBlockBuilder()`, a function to obtain an object which can efficiently iterate the chunks of the (main) graph from high to low chunk feerate, allowing each to be skipped or included.
* `TxGraph::GetWorstMainChunk()`, a function to obtain the last chunk that would be returned by `GetBlockBuilder()`'s returned object, intended for eviction.
ACKs for top commit:
monlovesmango:
reACK 8673e8f019
instagibbs:
reACK 8673e8f019
glozow:
reACK 8673e8f019
Tree-SHA512: 5c98c54919c44eb2f9545dfc130e54dfc25b5b54d43cf5ca9bcf46e019b9fd405a572fcd70e71e2a7c5b4b096cfd540a4d09ef1f52ba188504418682f1dfc4af
- Create a new function `AddMerkleRootAndCoinbase` that compute the
block's merkle root, insert the coinbase transaction and the merkle
root into the block.
`-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
This interface lets one iterate efficiently over the chunks of the main
graph in a TxGraph, in the same order as CompareMainOrder. Each chunk
can be marked as "included" or "skipped" (and in the latter case,
dependent chunks will be skipped).
This is preparation for a next commit which will introduce a class whose
objects hold references to internals in TxGraphImpl, which disallows
modifications to the graph while such objects exist.
Intended to make bitcoin command line features more discoverable and allow
installing new multiprocess binaries in libexec/ instead of bin/ so they don't
cause confusion.
Idea and implementation of this were discussed in
https://github.com/bitcoin/bitcoin/issues/30983
Co-authored-by: Sjors Provoost <sjors@sprovoost.nl>
These functions are just meant to serve the needs of the bitcoin wrapper
executable, and are intentionally not very general purpose so they can be
simple.
3bbdbc0a5e qt, docs: Unify term "clipboard" (Hennadii Stepanov)
Pull request description:
A translator on Transifex noticed:
> The term "system clipboard" appears twice. The term "clipboard" appears 10 times. Perhaps we could standardize on just saying "clipboard"?
This PR addresses this issue.
ACKs for top commit:
davidgumberg:
ACK 3bbdbc0a5e
pablomartin4btc:
ACK 3bbdbc0a5e
Tree-SHA512: 61a100f60890d81122a4b8ce3e2cb7d355c7fb643de3196573f7f9107c6f52fa0b3e7a4f743ce2833e8c67b9cdad3568b761d730fef5c9781f5e1c45252888c4
002b792b9a gui: decouple WalletModel from RPCExecutor (furszy)
Pull request description:
A more comprehensive fix for the issue described in #837.
Since the `WalletModel` class is unavailable when compiling without wallet support
`(-DENABLE_WALLET=0)`, the RPC executor class should not be coupled to it.
This decoupling ensures GUI compatibility with builds that omit wallet support.
This also drops an extra `#ifdef ENABLE_WALLET` block which is always good.
ACKs for top commit:
w0xlt:
Code Review ACK 002b792b9a
pablomartin4btc:
tACK 002b792b9a
BrandonOdiwuor:
tACK 002b792b9a
hebasto:
ACK 002b792b9a, I have reviewed the code and it looks OK.
Tree-SHA512: a8e6b7e9d88dd8e0ff5e2d0de91be2f85fd0559265267d3bf6cae5a37606cf1ab6bc7415d5817a11006008de362f2ca3557ba772b4e1bd9fbef5f564be3b53bb
ab878a7e74 build: simplify *ifaddr handling (fanquake)
Pull request description:
We really just want to skip this when building for Windows. So do that,
and remove the two header checks (we also already use both of these
headers, unguarded, in the !windows part of the codebase).
Squash the two *iffaddrs defines into one, as I haven't seen an
`iffaddrs.h` that implements one, but not the other.
ACKs for top commit:
hebasto:
ACK ab878a7e74. Only addressed my [comment](https://github.com/bitcoin/bitcoin/pull/32446#discussion_r2079994126) and rebased since my recent [review](https://github.com/bitcoin/bitcoin/pull/32446#pullrequestreview-2825606189).
TheCharlatan:
ACK ab878a7e74
Tree-SHA512: 7667305df9fef4728526c7217f85b51e739ec63b38e808da51d6ae65cb6f2696afa5ba82e5a72ed4a7a9b79ffa2402640448af4392587253027122eab7618e30
