This new optional replaces nBlockMaxWeight.
Use uint64_t for the block weight options to match BlockAssembler's
nBlockWeight accounting and the IPC schema's blockReservedWeight type.
A negative -blockmaxweight value is now parsed as 0 instead of an
overflowed signed value before validation rejects it.
The new block_max_weight option is not exposed to IPC clients.
Move the argument parsing for -blockmaxweight, -blockreservedweight,
-blockmintxfee out of init.cpp to a dedicated mining_args.cpp.
This is mostly a refactor and keeps the existing validation checks. It
does switch the weight arguments to GetArg<uint64_t>, introduced in
bitcoin/bitcoin#34582, so very large or negative weight values can be
reported differently in error messages.
Have most tests, benchmarks and fuzzers go through the mining interface.
This is a refactor: it does not change what blocks are created, just how
the creation calls are made. This avoids most direct test, benchmark and
fuzzer use of node::BlockAssembler::Options, making it easier to drop in a
later commit.
Two exceptions which use BlockAssembler directly:
- one check in test/miner_tests.cpp needs m_package_feerates
- fuzz/tx_pool.cpp Finish() doesn't have access to a NodeContext
Move test_block_validity from BlockAssembler::Options to
BlockCreateOptions so bench/block_assemble.cpp can continue to set it.
Just like coinbase_output_script, this is not exposed to IPC clients.
Inline options variable in places where it's only needed once.
We also drop one unused PrepareBlock declaration and one unused
implementation.
TestChain100Setup::CreateBlock no longer needs a chainstate argument,
which in turn means it can be dropped from CreateAndProcessBlock. Using
the Mining interface here also requires marking the test
KernelNotifications chainstate as loaded after LoadVerifyActivateChainstate().
The next commit switches test helpers that take a const NodeContext& to
create blocks through the Mining interface, so MakeMining needs to
accept a const NodeContext too.
Verify that -blockmaxweight is honored both when an IPC block template is first created and after waitNext() refreshes the template.
Co-authored-by: w0xlt <94266259+w0xlt@users.noreply.github.com>
Demonstrates that BlockTemplateImpl::waitNext() must respect the
mining policy supplied via -blockmintxfee, not silently fall back
to defaults.
Co-authored-by: Enoch Azariah <enirox001@gmail.com>
02b2c41103 logging: use util/log.h where possible (Anthony Towns)
57d7495fe5 IWYU fixes (Anthony Towns)
611878b46f scripted-diff: logging: Drop LogAcceptCategory (Anthony Towns)
34332dba2f util/log, logging: Provide ShouldDebugLog and ShouldTraceLog instead of a generic ShouldLog (Anthony Towns)
abea304dd6 logging: Move GetLogCategory into Logger class (Anthony Towns)
58113e5833 util/log: Rename LogPrintLevel_ into detail_ namespace (Anthony Towns)
f69d1ae56d util/log: Provide util::log::NO_RATE_LIMIT to avoid rate limits (Anthony Towns)
72e92d67df logging: Protect ShrinkDebugFile by m_cs (Anthony Towns)
904c0d07bb util/stdmutex: Drop StdLockGuard (Anthony Towns)
Pull request description:
`ShrinkDebugFile` now takes the logging mutex for its entire run; though it's only called in init so shouldn't have any races in the first place.
Adds a `NO_RATE_LIMIT` tag that can be used with info/warning/error logs to avoid rate-limiting. This allows `LogPrintLevel_` to be restricted to being an internal API.
The `GetLogCategory` function is moved out of the global namespace.
`ShouldLog` is split into separate `ShouldDebugLog` and `ShouldTraceLog` so that filtering checks are somewhat more enforced via function signature checks.
Redundant `LogAcceptCategory` function is removed.
More files are pointed at util/log.h instead of logging.h.
ACKs for top commit:
maflcko:
review ACK 02b2c41103📅
sedited:
Re-ACK 02b2c41103
l0rinc:
untested ACK 02b2c41103
ryanofsky:
Code review ACK 02b2c41103435d8dbaa77a526e484066471b2b8c! Overall a lot of nice improvements here.
Tree-SHA512: 3bffdca91afbe5c45a522815fe82e6f4cfa96529a4a243b29aad21234650502d6cac780126b584ee3e7ec129d8fdd50670d8a05036cc5c36e586b8c4c3563970
ac9aa71b7f mempool: remove all subsequent tx in pkg on failure (Greg Sanders)
Pull request description:
This belt-and-suspenders check, if ever hit in production, could result in an inconsistent mempool if somehow the parent failed in the ConsensusScriptChecks but the child did not. Rather than allow the mempool to get in an inconsistent state, remove the following txs in the package.
ACKs for top commit:
ismaelsadeeq:
Code review ACK ac9aa71b7f
marcofleon:
ACK ac9aa71b7f
sedited:
ACK ac9aa71b7f
Tree-SHA512: c25310045fa4dfd40bd38c9d54fff3f9fdb817e617154444d4691576393eee5f9cc86e26c59ddcdeef20eb6dddab0a168e91aec85c8291e6d68abbcb333d24f8
ca5483a662 qa: use NORMAL_GBT_REQUEST_PARAMS consistently (Antoine Poinsot)
Pull request description:
Functional tests have a constant that defines normal parameters to `getblocktemplate` but some tests were still hardcoding its value. This PR updates those tests to use the constant instead, so that if normal parameters to `getblocktemplate` need to be changed, it is only necessary to change them in a single place.
This is preparatory work for the implementation of BIP 54, which introduces a new GBT "forced" rule.
ACKs for top commit:
fanquake:
ACK ca5483a662
sedited:
ACK ca5483a662
Tree-SHA512: 07374e501ea66ba7b62f610797758fd3528950215b987a50cb18aa844bbf5fb5f9f49aafebc1472bf336de99ff96d1e74f550c09866fdf2882c73b87ac2a715e
7209eb7790 test: suppress ECONNABORTED in wait_for_rpc_connection on Windows (Ryan Ofsky)
Pull request description:
Since bitcoin/bitcoin#33362, `feature_bind_port_externalip.py` auto-detects whether 1.1.1.5 is available by starting a node with `-bind=1.1.1.5` and converting a bind failure into a skip. On Windows CI the address is not configured, so bitcoind exits as expected — but intermittently an RPC probe raises `ConnectionAbortedError` (WSAECONNABORTED/WinError 10053) while the process is shutting down, causing the test to fail rather than skip.
Fix by treating ECONNABORTED the same as the other transient connection errors already suppressed during startup (ECONNRESET, ETIMEDOUT, ECONNREFUSED).
Fixes#35343
ACKs for top commit:
maflcko:
lgtm ACK 7209eb7790
willcl-ark:
utACK 7209eb7790
hodlinator:
crACK 7209eb7790
Tree-SHA512: 04cf70a2d9247b32e9302827911b0d00a41aa4ac3cf0ba7a9c2aa86009019b14b3f914a3049e08b66b45ccd00530a77129aecf14a953007d28d3322f5ca16a70
Since bitcoin/bitcoin#33362, feature_bind_port_externalip.py runs in CI and
auto-detects whether 1.1.1.5 is assigned by starting nodes with -bind=1.1.1.5,
then converting a FailedToStartError containing "Unable to bind to" into a
SkipTest. On Windows CI the address is not configured, so bitcoind fails as
expected — but intermittently, while the process is shutting down, an RPC probe
raises ConnectionAbortedError (WSAECONNABORTED/WinError 10053) before
wait_for_rpc_connection() notices that the process has exited. That error was
not in the suppressed set, so it escaped and the test failed instead of being
skipped.
The intermittency is a race on TCP connection timing. If the probe connects
before the RPC port is listening, connect can fail with ECONNREFUSED. If the
TCP connection is established but is then closed abortively during shutdown,
the probe can see a connection-reset/aborted error instead. On POSIX systems
this commonly shows up as ECONNRESET ("connection reset by peer"). Winsock
also has WSAECONNABORTED for cases where an established connection is aborted
locally or otherwise terminated due to a timeout/protocol failure, in addition
to WSAECONNRESET for a reset by the remote side. This is why the
Windows-specific error may need to be suppressed separately.
Fix by treating ECONNABORTED identically to ECONNRESET, ETIMEDOUT, and
ECONNREFUSED: retry the probe rather than raising.
Fix was suggested by willcl-ark in
https://github.com/bitcoin/bitcoin/issues/35343#issuecomment-4507329622Fixes#35343
8ce84321ce musig: Reject empty pubkey list in GetMuSig2KeyAggCache (nervana21)
Pull request description:
Per [BIP327](https://github.com/bitcoin/bips/blob/master/bip-0327.mediawiki?plain=1#L300), MuSig2 key aggregation is defined for `u` public keys where `0 < u < 2^32`.
Previously, the code did not handle `u == 0`.
This patch updates the code to reject an empty pubkey list and adds a regression test.
ACKs for top commit:
achow101:
ACK 8ce84321ce
rkrux:
lgtm ACK 8ce84321ce
Tree-SHA512: aa662eee92b6c637683b8535fd9e62431538ba58f38d32f4538259f0b534793625b74a3a10c0c5bb23a7ec7ec83925170b23e47574ef4ebde283a18a0b143d86
b63ef20d54 test: add fuzz harness for CDBWrapper (Andrew Toth)
32169c3855 dbwrapper: accept optional testing leveldb::Env in DBParams (Andrew Toth)
8d390c93fc dbwrapper: make max_file_size a configurable DBParams field (Andrew Toth)
Pull request description:
Inspired by https://github.com/bitcoin/bitcoin/pull/34866#issuecomment-4090291488.
We currently don't have a dedicated harness targeting `CDBWrapper`. OSS-Fuzz has a [rudimentary harness](https://github.com/google/oss-fuzz/blob/master/projects/leveldb/fuzz_db.cc) for levelDB [which fails](https://issues.oss-fuzz.com/issues/447252244), so doesn't appear maintained.
This PR adds a harness targeting `CDBWrapper` against an in-memory oracle to verify correctness.
A `DeterministicEnv` wraps levelDB's `memenv` to eliminate non-determinism by capturing background compaction and running it at fuzzer-chosen points.
The fuzzer also controls the cache_bytes and max_file_size sizes so that small values trigger memtable flushes and compaction.
ACKs for top commit:
l0rinc:
code review ACK b63ef20d54
marcofleon:
ACK b63ef20d54
dergoegge:
utACK b63ef20d54
sedited:
ACK b63ef20d54
Tree-SHA512: da1f738ec90c49830a05b8990bdaa474299b573e966e60f4febef1292d9682f2e50f0016831f26bf4677e5afdaa142dc8766d871c6bce90d35f1695d480ac8c1
eff9e798b9 coinselection: Tiebreak SRD eviction by weight (Murch)
Pull request description:
yancyribbens [pointed out](https://github.com/p2pderivatives/rust-bitcoin-coin-selection/pull/108#issuecomment-3202107069) that SRD would fail to find a possible solution if there are multiple UTXOs of the same effective value with diverse weight.
This adds a tiebreaker that will make SRD succeed in such a scenario.
ACKs for top commit:
yancyribbens:
utACK eff9e798b9
Tree-SHA512: 6a26f3c06f3346cef7e06926d9f747ecea91f057435f22fa8c3333f129227cf5a361f45047003e8a762a99225e3df27e3980a8b4397b36dc9b59c44a9626a2ec
90eda67bb8 Remove opt-in RBF (Pol Espinasa)
Pull request description:
With the whole network mostly running with Full Replace By Fee, it does not make sense to let the user signal or not to signal for RBF on their transactions.
With this PR the transactions created using the GUI will fallback to the wallet configuration in order to signal or not to signal BIP 125. (True by default).
ACKs for top commit:
achow101:
ACK 90eda67bb8
sedited:
ACK 90eda67bb8
Tree-SHA512: 6253501b89ae509eb5bf3c6c81aaf117f03ef2ba411aa3b0a6f05bca07cbf7a1030c519f8825eb2d7269cc286a42ed17c0becccc04ad252bcc9c868086ff4cdc
3142e5f8cf doc: Add release notes for #32220 (Hennadii Stepanov)
b71cd5c162 cmake: Skip using git when building from source tarball or as subproject (Hennadii Stepanov)
fe941938e8 cmake: Remove unnecessary `BITCOIN_GENBUILD_NO_GIT` environment variable (Hennadii Stepanov)
9a2cced23a cmake, refactor: Move `find_package(Git)` to `src/CMakeLists.txt` (Hennadii Stepanov)
Pull request description:
In general, the Bitcoin Core build system attempts to fetch commit or tag details from git. This is handled by the [`cmake/script/GenerateBuildInfo.cmake`](https://github.com/bitcoin/bitcoin/blob/master/cmake/script/GenerateBuildInfo.cmake) script, which generates the [`src/bitcoin-build-info.h`](65dcbec756/src/clientversion.cpp (L26-L31)) header within the build tree.
However, there are cases where the retrieved details may be incorrect—for example, when building from a source tarball or as a subproject within a git-aware project.
In the Autotools-based build system, the `BITCOIN_GENBUILD_NO_GIT` environment variable was [introduced](https://github.com/bitcoin/bitcoin/pull/7522) in [v0.20.0](https://github.com/bitcoin/bitcoin/blob/master/doc/release-notes/release-notes-0.20.0.md) to address such scenarios:
> The process for generating the source code release ("tarball") has changed in an effort to make it more complete, however, there are a few regressions in this release:
> - Instead of running `make` simply, you should instead run `BITCOIN_GENBUILD_NO_GIT=1 make`.
This PR automagically handles both of the aforementioned cases and removes the need for `BITCOIN_GENBUILD_NO_GIT`.
The user is still able to configure the build with [`-DCMAKE_DISABLE_FIND_PACKAGE_Git=ON`](https://cmake.org/cmake/help/latest/variable/CMAKE_DISABLE_FIND_PACKAGE_PackageName.html) to disable git execution manually, for [reasons](https://github.com/bitcoin/bitcoin/pull/32220#issuecomment-2780115034) we don't know in advance.
Closes https://github.com/bitcoin/bitcoin/issues/31999.
ACKs for top commit:
maflcko:
review ACK 3142e5f8cf🥄
fanquake:
ACK 3142e5f8cf - tested a few different build scenarios (from Guix the tarball), they all seem to work as intended.
Tree-SHA512: 14995dc76fc680cff2bbc983d525125b39f49397e8ed94fffab29de505723a8e2f7f261c1a7ca88d98776755ccf63e59a32c476e112683f5cc1c016a7cfede94
fa3d7ce11c doc: Document minimum versions for Xcode CLT and MSVC (MarcoFalke)
Pull request description:
The minimum required Xcode command-line tools version was not documented, which can lead to confusion.
So document it in `doc/dependencies.md` (along with adding a note one msvc there as well). This also allows to slim down the error message in the configure C++ feature check by referring to `doc/dependencies.md#compiler`.
ACKs for top commit:
polespinasa:
reACK fa3d7ce11c
l0rinc:
ACK fa3d7ce11c
Tree-SHA512: daa594e39c94f615888b2dfbf7cb8d9f7d16e5539f18a6ee57686b8364de27533f603c2324c5e213504d55a2dce86469254bfce5c0d2af7be059348bceb1d25b
98f706c698 qa: regenerate hardcoded regtest chain for kernel lib unit tests (Antoine Poinsot)
Pull request description:
The kernel library uses a harcoded regtest block chain in the unit tests. This chain was generated prior to #32155 and its coinbase transactions are not BIP 54 compatible.
This PR updates the hardcoded chain to be BIP 54 compatible and contain more transactions. It was generated based on the chain from the `wallet_migration.py` functional test as it contains coins for a wide variety of output types (though it may be useful to add more transactions *spending* from these output types). Some trivial hardcoded values in the unit test had to be updated to match the new chain.
ACKs for top commit:
alexanderwiederin:
ACK 98f706c698
sedited:
ACK 98f706c698
Tree-SHA512: df3370f17e158b36a072877d13f0ee68987fd4002f870e7590e2a24e865e474459b2682ccbdad54934acc452296560c73378597370767f4a557375c594d3c105
6189335f6b kernel: doc: document wipe lifecycle and best entry nullability (csjones)
Pull request description:
Document on `btck_chainstate_manager_options_set_wipe_dbs` that a wipe must be followed by `btck_chainstate_manager_import_blocks` before the chainstate manager is used for anything else, as the existing kernel tests already do (specifically the `chainman_reindex*` kernel tests). Note in the `@return` of `btck_chainstate_manager_get_best_entry` that it can return null when no block headers have been loaded.
Background: I've been working on a bindings project using the libbitcoinkernel and tripped on a SIGSEGV calling the entry accessors on the null pointer returned by `get_best_entry` after a `(true, true)` wipe (#35293). The C++ wrapper handles this via `btck::check<>`, but bindings generated from the C header don't see the wrapper. Adding a nullability documentation hint helps generators produce the correct signature, and the `@note` on `set_wipe_dbs` documents the lifecycle that avoids the null in the first place.
Docs-only change; no tests ran.
ACKs for top commit:
sedited:
ACK 6189335f6b
alexanderwiederin:
ACK 6189335f6b
Tree-SHA512: 319d9704c9857c8eb12e8726d16710d8b5777404f8ecf50d832a5e5b0a9b9f0d8321074ecb8b985e5b03a6eb7119cc4eb73f36d096dc0149a73de7d6a74de4cb
This belt-and-suspenders check, if ever hit in production,
could result in an inconsistent mempool if somehow the
parent failed in the ConsensusScriptChecks but the child did
not. Rather than allow the mempool to get in an inconsistent
state, remove the following txs in the package.
Introduces a libFuzzer harness that exercises CDBWrapper operations
against a std::map oracle, with a DeterministicEnv that captures LevelDB
background compaction for single-threaded determinism.
A sibling dbwrapper_threaded target uses a bare memenv so LevelDB's real
background thread runs, exercising force_compact and threaded compaction
paths that the deterministic variant cannot reach.
Adds an implicit-integer-sign-change suppression for
BytewiseComparatorImpl::FindShortSuccessor (leveldb/util/comparator.cc:58)
to the test ubsan suppressions list. LevelDB's bytewise comparator
implicitly converts a signed `char` byte to `uint8_t` there. The path
is only reached when compaction picks an SST boundary key, so it
requires a small enough max_file_size for compaction to fire during
the fuzz run.
Co-authored-by: l0rinc <pap.lorinc@gmail.com>
Allow callers to inject a custom leveldb::Env via DBParams::testing_env,
which takes priority over the memory_only in-memory environment. This
enables fuzz harnesses to supply a deterministic environment.
cd8d3bd937 wallet: use outpoint when estimating input size (Lőrinc)
Pull request description:
### Problem
`CalculateMaximumSignedInputSize()` is passed the outpoint being sized, but a previous refactor stopped using that context when estimating the signed input size.
This could make externally selected inputs look slightly smaller than they really are.
### Fix
Pass the outpoint through again when estimating the signed input size.
Add a regression test for the external-input case.
> [!NOTE]
> the branch name still reflects the previous state of this PR, where the unused parameter was removed instead of wired back in
ACKs for top commit:
achow101:
ACK cd8d3bd937
pablomartin4btc:
ACK cd8d3bd937
Tree-SHA512: 6089ae65ae12677c32be0556d704f8c179f1ff5a017690846ae495644890526f85d8c0d75d4ec4c3c9ac5b519251169009484623340b8bc3a87fa9a3be27fefd
3381855e51 doc: external signer: update interface, --stdin flag, IPC-command signtx, contains updates from #33947 (Danny van Heumen)
Pull request description:
Updates to documentation for External Signer.
- Added mention that `signtransaction` command is no longer primary mechanism.
- Document inter-process communication via `--stdin` flag followed with stdin-content.
- Document `signtx` command followed by Base64-encoded PSBT.
ACKs for top commit:
Sjors:
ACK 3381855e51
naiyoma:
ACK 3381855e51
Tree-SHA512: e9c666c7a9de08a148846c8d2d1fc2905ba7ce672b7baad35fd9d7a693bfd9beae99e29134aa24282fc14d2de86bbf653ad15e167658a075d4ec9f5bcdbaabdd
ae73b69b52 test: restore assertion that tx contains exactly 2500 sigops (ismaelsadeeq)
Pull request description:
darosior wrote https://github.com/bitcoin/bitcoin/pull/29060#discussion_r3267762329:
_This is useful documentation, plus useful in making sure the comment above the check does not become stale or incorrect._
Hence reverted.
ACKs for top commit:
l0rinc:
code review ACK ae73b69b52
sedited:
ACK ae73b69b52
willcl-ark:
ACK ae73b69b52
Tree-SHA512: 2c76e9b66e367613c1232b65b1c18f2d0c1068acdf712ca0937dae465e637b024df95d6479f26cc5d04e5767e711eb5b3f3a329207af75ee64c7bfc8bc9173f6
0429c503fb bench: Replace Coin Selection bench (Murch)
ec1eefda77 bench: Remove unnecessary wallet parameter (Murch)
e6c4ffb956 bench: Fix type mismatch (Murch)
Pull request description:
Adds a Coin Selection benchmark that doesn’t just test a worst case of one of the algorithms but exercises coin selection to to select inputs for a variety of different targets from a large number of UTXOs.
ACKs for top commit:
l0rinc:
code review ACK 0429c503fb
sedited:
ACK 0429c503fb
Tree-SHA512: 53238d39c8f6d543d80af77e3bb23ab418f2ee266a5ae407fd739c158ca86db553457dcc372b7aa5017f392fb5ae784394cad9edd79b1c0f58ffc32c89e0c306
0358c26d42 kernel: document overwritten validation state outputs (w0xlt)
Pull request description:
This PR updates the public kernel API documentation for validation-state output parameters that are still caller-provided:
- `btck_transaction_check`
- `btck_block_check`
Both wrappers reset the supplied validation state on entry before running validation, so callers should treat the state as overwritten in-place rather than preserving prior contents.
ACKs for top commit:
yuvicc:
re-ACK 0358c26d42
sedited:
ACK 0358c26d42
Tree-SHA512: f0097c38449c09c6c614a1fb6e5fe09bc84e5dae57c0cb57540419fd6c3f40c06ce8b41e12ab2eff27f4b18d053d32aba2c4a7551a037be93d618b1734922f37
Document btck_transaction_check and btck_block_check validation state output parameters as overwritten in-place. This matches their reset-on-entry behavior and avoids implying callers should preserve prior state.
88d9bc5aa4 kernel: Return btck_BlockValidationState from process_block_header API (yuvicc)
Pull request description:
This PR refactors `btck_chainstate_manager_process_block_header` to return `btck_BlockValidationState` by value instead of using out-parameters or boolean returns.
ACKs for top commit:
optout21:
ACK 88d9bc5aa4
stickies-v:
ACK 88d9bc5aa4
w0xlt:
reACK 88d9bc5aa4
hodlinator:
re-ACK 88d9bc5aa4
Tree-SHA512: f86b6e85aedafd78ae250930cbe34dc666c14d800e43cf8582d49aecb97faab801eff8dcc0250082ceebc3e8d32949839e030cf9f0023b56b23c8f7b7a741e49
75cf9708a0 ci: add one more routable address to the VMs (docker containers) (Vasil Dimov)
1b93983bf5 test: make feature_bind_port_(discover|externalip).py auto-detect the skip condition (Vasil Dimov)
Pull request description:
`feature_bind_port_discover.py` and `feature_bind_port_externalip.py` require a routable address on the machine to run. Since that was not predictably available on CI, those tests required a manual setting up of IP addresses (e.g. using `ifconfig`) and then running the tests with a command line option telling them that the addresses are set up. The tests were not run in CI and [got rot](https://github.com/bitcoin/bitcoin/issues/31293#issuecomment-2497792487).
Change that to auto-detect, from the tests, whether the needed IP addresses are present and if yes, run the test, otherwise skip it. Also change the CI to configure the needed addresses when running the functional tests. This way the tests will be run regularly on CI.
Fixes: https://github.com/bitcoin/bitcoin/issues/31336
ACKs for top commit:
willcl-ark:
ACK 75cf9708a0
frankomosh:
Tested ACK 75cf9708a0. Built from source.
ryanofsky:
Code review ACK 75cf9708a0. Tested locally with and without the special addresses, and the detection seems to work well.
Tree-SHA512: 252911a37a06764f644a1a83c808f5255ac3bc74919426afa5d082c59e1ea924196354735f229d381cb5aff2340e001c2240bbadc8b5f27e5321fb4cfaef0fdb
81348576cc psbt, test: remove address type restrictions in test (rkrux)
Pull request description:
Because the corresponding Taproot fields were added in PSBT in #22558,
so these restrictions are no longer necessary.
ACKs for top commit:
kevkevinpal:
ACK [8134857](81348576cc)
polespinasa:
lgtm ACK 81348576cc
furszy:
utACK 81348576cc
Tree-SHA512: 5621509e103674ee5f39454871ca5acb2bf4c16b85dbbdc38abac22fbaea44cafe4bbd91dc3a5c6435b3859a4dc58f12bf7aeec2952c9f80fbbdad2adb52847c
Remove redundant int return from btck_chainstate_manager_process_block_header.
Previously returned both an int result and an output validation state parameter, creating ambiguity
where non-zero could mean either invalid header or processing failure. Since ProcessNewBlockHeaders
already provides complete validation info, the int return was redundant.
Co-authored-by: stringintech <stringintech@gmail.com>
Co-authored-by: stickies-v <stickies-v@protonmail.com>
Co-authored-by: Hodlinator <172445034+hodlinator@users.noreply.github.com>
1c500b1709 test: avoid non-loopback network traffic from node_init_tests/init_test (Vasil Dimov)
Pull request description:
The test `node_init_tests/init_test` calls:
`AppInitMain()` -> `StartMapPort()` -> `StartThreadMapPort()` -> `ThreadMapPort()` -> `ProcessPCP()` -> `PCPRequestPortMap()` -> `CreateSock()` and on the returned value from `CreateSock()` it calls the `Connect()` method.
Thus, change `BasicTestingSetup::BasicTestingSetup()` to set `-natpmp` to 0. This way `node_init_tests/init_test` or other tests will not do network activity due to `ThreadMapPort()`.
Also add a comment about `natpmp=0` in
`test/functional/test_framework/util.py`.
Also set `-dnsseed=0` in `BasicTestingSetup::BasicTestingSetup()` to
avoid DNS queries.
ACKs for top commit:
fjahr:
re-ACK 1c500b1709
ryanofsky:
Code review ACK 1c500b1709, just disabling -dnsseed since previous review, which makes sense.
Tree-SHA512: 3b275d91361804da6d1dc109dffe741ea4b3dd3be916eb12fa63efa233e13ea4dab5a9f8448bd8bf99bc41817f3b7a768abe91531a3f63eae8b4c912bfcbd13e
89af67d79f tests: Add some fuzz test coverage for command-specific args (Anthony Towns)
92df785859 tests: Add some test coverage for ArgsManager::AddCommand (Anthony Towns)
33c8090be9 ArgsManager: automate checking for correct command options (Anthony Towns)
186354a0d8 bitcoin-wallet: use command-specific options (Anthony Towns)
d21e82b7d6 ArgsManager: support command-specific options (Anthony Towns)
Pull request description:
Adds the ability to link particular options to one or more (`OptionsCategory::COMMANDS`) commands, and uses this feature
in `bitcoin-wallet`. Separates out the help information for these command-specific options (duplicating it if an option applies to multiple commands), and provides a function for checking at runtime if some options have been specified by the user that only apply to other commands.
#### Motivation
Currently, `ArgsManager` supports commands like `bitcoin-wallet dump` but while some of the options are command-specific (like `-dumpfile`), `ArgsManager` itself doesn't know that. As a result, `-dumpfile` is listed in the global help rather than under the relevant commands, and if you use `-dumpfile` with a different command that doesn't support it, `ArgsManager` cannot automatically report that as an error, resulting in the commands that don't support the option having to have error-handling specific to all the options they don't support.
#### Changes
Help output moves command-specific options under their associated commands:
Before:
```
Options:
-dumpfile=<file name>
When used with 'dump', writes out the records to this file. When used
with 'createfromdump', loads the records into a new wallet.
...
Commands:
createfromdump
Create new wallet file from dumped records
dump
Print out all of the wallet key-value records
```
After:
```
Commands:
createfromdump
Create new wallet file from dumped records
-dumpfile=<file name>
When used with 'dump', writes out the records to this file. When
used with 'createfromdump', loads the records into a new wallet.
dump
Print out all of the wallet key-value records
-dumpfile=<file name>
When used with 'dump', writes out the records to this file. When
used with 'createfromdump', loads the records into a new wallet.
```
Error messages are now generated automatically by `ArgsManager` rather than ad-hoc wallet code for each option:
Before:
```c++
if (args.IsArgSet("-dumpfile") && command != "dump" && command != "createfromdump") {
tfm::format(std::cerr, "The -dumpfile option can only be used with the \"dump\" and \"createfromdump\" commands.\n");
return false;
}
```
After:
```c++
std::vector<std::string> details;
if (!args.CheckCommandOptions(command, &details)) {
tfm::format(std::cerr, "Error: Invalid arguments provided:\n%s\n", util::MakeUnorderedList(details));
return false;
}
```
#### Limitations
- If an option applies to multiple commands, it shares the same help text. There's no way to provide per-command descriptions.
- Option parsing rules are unchanged — options still cannot appear after the command.
ACKs for top commit:
achow101:
ACK 89af67d79f
sedited:
Re-ACK 89af67d79f
ryanofsky:
Code review ACK 89af67d79f. Since last review: rebase, integration with ClearArgs and fuzz test, and Assume -> Assert switch
Tree-SHA512: 7ae7c3b74d0c8c4db8459e9f0b9c7498b2fa4758954ec49983decbba177877b039779f0f7b55e60c3a0ed74c5e9e4ac4734ba9e049bf3a7743280ef8300869fa
da769855d0 test: add PSBT proprietary merge regression coverage (w0xlt)
3f5b3c7a80 psbt: preserve proprietary fields when combining PSBTs (w0xlt)
Pull request description:
BIP 174 proprietary fields are currently parsed, serialized, and exposed by `decodepsbt`, but they are not preserved by `combinepsbt`.
The reason is that the merge paths in `PartiallySignedTransaction::Merge()`, `PSBTInput::Merge()`, and `PSBTOutput::Merge()` union `unknown`, but never union `m_proprietary`.
This means application-specific PSBT metadata can be lost during combination, even though BIP 174 treats proprietary records as normal PSBT key-value pairs for private or application-specific use.
This PR fixes that by preserving proprietary fields in all three merge paths.
ACKs for top commit:
nervana21:
re-ACK da769855d0
Bicaru20:
re-ACK da769855d0
achow101:
ACK da769855d0
theStack:
Code-review ACK da769855d0
Tree-SHA512: 4474674ac00c3155fd7d3d777bdeb70d4ca2006c8fe62eb84a888ef2f7aa02739bf31d8e9f2cb59e324be1085c77897f0bbf6673254c12408ab09a1582e28b83
b6c3670442 i2p: clean up SAM error logging (takeshikurosawaa)
Pull request description:
Clean up the I2P SAM error path.
`SESSION CREATE` may contain the private key, so the generic SAM reply
error path now reports the redacted request text instead of the full
request. It also avoids echoing raw router replies in those generic
error messages.
No network behavior change intended.
ACKs for top commit:
davidgumberg:
crACK b6c3670442
vasild:
ACK b6c3670442
Tree-SHA512: 204c8b64c6d3dd2f94f92cdc6d3daefd7773c42066984b9da859ebc2912c2ed38079d9e82a2d1f09d8d720750047114a80189e688929d7a0af5da2c2ee4a88da
8544537f41 mining: drop unused include_dummy_extranonce option (Sjors Provoost)
58eeab790d mining: only pad with OP_0 at heights <= 16 (Sjors Provoost)
00d22328b0 mining: pad coinbase to fix createNewBlock at heights <=16 (Sjors Provoost)
605ff37403 test: bad-cb-length for createNewBlock() at low heights (Sjors Provoost)
1966621b76 test: refactor IPC mining test to use script_BIP34_coinbase_height (Sjors Provoost)
Pull request description:
Blocks 0-16 on any new chain require mining code to be careful not to violate the `bad-cb-length` rule, which states the coinbase transaction scriptSig must be at least 2 bytes.
Our mining code deals with that by padding the `scriptSig` with a 0 `extraNonce`. It does this for every height. As a result IPC clients would get an unnecessary `0` in the `scriptSigPrefix` field of `CoinbaseTx`. #32420 fixed that by introducing a `include_dummy_extranonce` option in `BlockCreateOptions` and turning that off for IPC clients.
A minor issue was missed though: `createNewBlock()` now fails with `bad-cb-length`. An easy workaround is to use the `generate` RPC for the first 16 blocks, as demonstrated in the 2nd commit.
The real fix is to have the miner code always pad the `scriptSig` at lower heights, but to _not_ include that in the `scriptSigPrefix` field of `CoinbaseTx` (introduced in #33819). This is what the 3rd commit implements.
Now that we set `scriptSigPrefix` independent of what our internal miner code does - to get past `CheckBlock()` - the original motivation for `include_dummy_extranonce` goes away and we can just drop it entirely. The last commit drops it, while the 4th commit adjusts the tests and hardcoded block and assume utxo hashes.
This last change does not break IPC clients, because `include_dummy_extranonce` was never exposed in `mining.capnp`.
Instead of adjusting the hardcoded hashes, an alternative approach would be to just always pad the `scriptSig` internally, since we exclude the padding from `scriptSigPrefix` anyway. However, IPC clients can also call `getBlock()` to get the raw block and might be confused about the difference. The miner code is also easier to understand if we limit the exception (`coinbase_tx.script_sig_prefix != coinbaseTx.vin[0].scriptSig`) to `nHeight <= 16`, where the explanation is based purely on consensus rules rather than historical test suite reasons.
The first two commits are preperation test changes:
- extract `assert_capnp_failed` helper for macOS (also part of #34727)
- use `script_BIP34_coinbase_height` in IPC mining test (existing code in `interface_ipc_mining.py` was incorrect for low height
Fixes#35126
ACKs for top commit:
ryanofsky:
Code review ACK 8544537f41. Just rebased to fix silent conflict and applied some minor suggestions since last review. As part of rereviewing I left some more minor suggestions that are fine to ignore.
sedited:
Re-ACK 8544537f41
Tree-SHA512: a01d48842bf4bcc1a9c51a89ef9d750766db7d04edb4dcd6b3a8bf195c6b4fa07445256a49367ff0db00ab489a52a3d7ff6a5c3ab9290ecb1fcb82f532552e9b
CombinePSBTs currently preserves unknown records but drops proprietary records at the global, input, and output levels because the Merge() paths never union m_proprietary.
Preserve proprietary records in PartiallySignedTransaction::Merge(), PSBTInput::Merge(), and PSBTOutput::Merge() so combine/merge keeps all PSBT key-value data.
Document on `btck_chainstate_manager_options_set_wipe_dbs` that a wipe must be followed by `btck_chainstate_manager_import_blocks` before the chainstate manager is used for anything else, as the existing kernel tests already do (e.g. `chainman_reindex_test`).
Note in the `@return` of `btck_chainstate_manager_get_best_entry` that it can return null when no block headers have been loaded.
Refactor the `@return` documentation to fit on a single line.
