bfbf1a7ef3 kernel: Expose btck_transaction_check consensus function (w0xlt)
Pull request description:
This PR exposes the consensus-level `CheckTransaction` function through the libbitcoinkernel C API and adds a corresponding C++ wrapper.
Currently, libkernel only provided script-level validation via `btck_script_pubkey_verify` and `ScriptPubkeyApi<>::Verify`.
AFAIK there was no way to perform context-free consensus checks on a transaction’s structure (e.g., coinbase rules, money-range, duplicate inputs).
This change introduces a new API:
```c
int btck_check_transaction(const btck_Transaction* tx, btck_TxValidationState** out_state);
```
and a C++ convenience wrapper:
```cpp
std::pair<bool, TxValidationState> btck::CheckTransaction(const Transaction& tx);
```
Both follow the ownership and error-handling conventions established in `bitcoinkernel.h`.
The test suite is extended with cases covering:
* coinbase scriptSig length bounds
* empty vin / vout detection
* negative or out-of-range output values
* duplicate inputs
* null prevouts in non-coinbase transactions
ACKs for top commit:
alexanderwiederin:
Re-ACK bfbf1a7ef3
sedited:
Re-ACK bfbf1a7ef3
janb84:
ACK bfbf1a7ef3
Tree-SHA512: fe9ba555564af3258d9ccaea1b208df0aa2a4bf2fe74a8f861b3a3b3b891f2990ffc0e3b763d14f15568e977b70ee7337a5dc7d469c63098e0b43adc2ad80e3c
11713c9fa9 net: make CConnman::m_nodes_mutex non-recursive (Vasil Dimov)
aec4fa2de0 net: drop the only recursive usage of CConnman::m_nodes_mutex (Vasil Dimov)
Pull request description:
The only case of a recursive lock was a nested `ForNode()` call to trim
the size of `lNodesAnnouncingHeaderAndIDs` to `<= 3`. This need not be
nested, so take it out.
Before:
```
fornode(newnode)
if (size >= 3)
fornode(front) handle removal of front
pop front
push back newnode
```
After:
```
fornode(newnode)
push back newnode
if (size > 3)
fornode(front) handle removal of front
pop front
```
`lNodesAnnouncingHeaderAndIDs` is protected by `cs_main` which is locked
during the entire operation.
Partially resolves: https://github.com/bitcoin/bitcoin/issues/19303
---
_This PR included https://github.com/bitcoin/bitcoin/pull/32326 (first 3 commits in this PR). That PR was merged first, so the size of this was reduced._
ACKs for top commit:
Crypt-iQ:
utACK 11713c9fa9
w0xlt:
ACK 11713c9fa9
hodlinator:
re-ACK 11713c9fa9
sedited:
ACK 11713c9fa9
Tree-SHA512: 5c626423b5041bfecd8487e2201bfa29443aa2ebe955aa7dc6680e1ee4a094296c9034a6ad70d4d63046627edeeda87f73bd27059e08ae4fe20653d8ae380b71
eed7af666b doc: Add release note for disallowing some wallet path names (David Gumberg)
3d7f0e4ed5 wallettool: Use GetWalletPath to determine the wallet path (Ava Chow)
2b0dc0d228 wallet: Disallow . and .. from wallet names (Ava Chow)
Pull request description:
Wallet names including `..` and `.` are unintuitive and can lead to various issues, see #34497
This disallows creating or loading wallets that have any path elements that are `..` or `.`, including any present in an absolute path. This does not disallow relative paths altogether but rather limits them to only being subdirectories of the wallets directory.
ACKs for top commit:
davidgumberg:
crACK eed7af666b
w0xlt:
ACK eed7af666b
arejula27:
ACK eed7af666b
Tree-SHA512: bec5e54369061eb630d9afb94701badce09e8beb63686cf714016466fc01653d4841030fc10fcd14d44e6b1022c0994cb32253d80533807704d9e11eda2423ff
1d1ae6f0c4 wallet, test: Remove DuplicateMockDatabase (Ava Chow)
57820c472b bench: Utilize setup() for WalletLoading and use a real database (Ava Chow)
9a7604fd25 bench: Use setup() in WalletMigration to prepare the legacy wallet (Ava Chow)
426a94e7bd bench: Utilize setup() in WalletEncrypt to create the encryption wallet (Ava Chow)
d672455d20 bench: Utilitze setup() in WalletBalance for marking caches dirty (Ava Chow)
61412ef887 bench: Utilize setup() in WalletCreate to cleanup previous wallets (Ava Chow)
Pull request description:
Several of the wallet benchmarks have some setup or cleanup that needs to be done per run. Now that #34208 is merged, these can use `setup()`. Additionally, this allows for removing `DuplicateMockDatabase` in `WalletEncryptDescriptors`.
This PR also removes `DuplicateMockDatabase` in `WalletLoadingDescriptors`. `DuplicateMockDatabase` was added here in #24924 as part of benchmark performance improvements. However, it does not appear to make a significant difference today.
Removing `DuplicateMockDatabase` makes future database changes easier. In particular it should simplify #33032 and #33034, and any future changes that introduce sqlite features.
ACKs for top commit:
l0rinc:
code review ACK 1d1ae6f0c4
furszy:
Other than that, ACK 1d1ae6f0c4
sedited:
ACK 1d1ae6f0c4
Tree-SHA512: 41130144972b759b401f990820eaf524d1f17f47d81bf1afea4a529d15a21d253521838a9e31df8f424996582b718a92634ab255204c6fce703b7e47a1d23670
758f208cc1 contrib: override system locale in gen-manpages.py (Sjors Provoost)
Pull request description:
`bitcoin-qt --help` emits a translation of "version", which creates a diff when updating or verifying man pages.
The script aborts earlier however, because `bitcoin-qt --version` also emits a localized output, which triggers the `Copyright (C)` assertion on a translated term like "Auteursrecht".
Fix this by passing `--lang=en` to both `bitcoin-qt` invocations.
None of the actual command options are translated, so this commit does not affect the actual manual page.
Noticed while verifying the manual updates in #34800 on macOS with Dutch system locale.
See also https://github.com/bitcoin/bitcoin/blob/master/test/lint/lint-locale-dependence.py notes about localization (though the issue here is translation).
ACKs for top commit:
achow101:
ACK 758f208cc1
sedited:
ACK 758f208cc1
hebasto:
ACK 758f208cc1.
Tree-SHA512: 0947391b85a637ae6d1d0cf4b8de4ab74b42e772bf7d70991cf2b4e035c2cb99f0f46c6cb2cd8aac9cc1be22a9e8329594536688622d3bacab22289c8151e89d
938312d7a6 docs: clarify RPC credentials security boundary (crStiv)
Pull request description:
Explicitly states that RPC credentials grant full administrative access to the node and filesystem resources accessible by bitcoind. Adds a new section in JSON-RPC-interface.md to address issue https://github.com/bitcoin/bitcoin/issues/32274 by documenting that providing RPC credentials to untrusted clients
reopened#32424
P.S. I've tried to somehow squash all the commits from the previous pr but accidentally closed the pr and had no idea how to return back, therefore created a new pr, I'm really sorry for the inconvenience
ACKs for top commit:
achow101:
ACK 938312d7a6
janb84:
re ACK 938312d7a6
sedited:
ACK 938312d7a6
Tree-SHA512: 54db0651cfe4a92d700d09c822db5cb68f60f17a4634eb8f132939294e7a0ca2aea15ddc4d581610976158f7546e9c4463cfe113de9500162a0f107e168833cd
2104282ddd fuzz: Add tests for CCoinControl methods (Chandra Pratap)
43b09b993d fuzz: Improve oracle for existing CCoinControl tests (Chandra Pratap)
Pull request description:
The `ccoincontrol` fuzzer misses tests for a number of `CCoinControl` operations. Add them.
While at it, improve the oracle for the existing tests.
ACKs for top commit:
l0rinc:
Lightly tested code review ACK 2104282ddd
brunoerg:
reACK 2104282ddd
sedited:
Re-ACK 2104282ddd
Tree-SHA512: bfc8c9a51fca94437332056c476840d841a5b42dd6749cb34105b7ae78215ec9c3eb0f407e1a5f51b3ac20d7abb97cae7c21ad2146d5be9409edbc2cd2c568ee
Add btck_transaction_check() to the libbitcoinkernel C API, exposing
context-free transaction consensus validation (consensus/tx_check.h).
Introduces btck_TxValidationState with introspection and lifecycle
functions. btck_TxValidationResult is exposed for compatibility with
existing validation-state APIs, though btck_transaction_check currently
reaches only UNSET and CONSENSUS.
Includes C++ wrapper and test coverage for btck_transaction_check using
test vectors from tx_valid.json / tx_invalid.json.
1950da94fc test: enable `rpc_bind` on macOS and BSD (Lőrinc)
7236a05503 test: enable `feature_bind_extra` on macOS and BSD (Lőrinc)
Pull request description:
### Problem
Some functional tests are shown as skipped when running on macOS & BSD because `test_framework/netutil.py` only implemented the Linux-specific logic for checking which TCP sockets a node is listening on.
### Fix
Add macOS and BSD implementations in `test/functional/test_framework/netutil.py` so tests can query:
* which TCP sockets a node is listening on (`get_bind_addrs()`, via `lsof`)
* a non-loopback interface address (`all_interfaces()`, via `ifconfig`)
Then enable the previously Linux-only tests by switching to a shared POSIX platform guard.
### Commands
<details>
<summary><code>get_bind_addrs()</code> (<code>lsof</code> + regex)</summary>
> Command used
```bash
lsof -nP -a -p <pid> -iTCP -sTCP:LISTEN -Ftn
```
> Flags
- -D: device cache warnings
- -n: no hostname resolution
- -P: no service/port-name resolution
- -a: AND all conditions
- -p <pid>: filter by process ID
- -iTCP: TCP sockets only
- -sTCP:LISTEN: listening sockets only
- -Ftn: machine-readable output (fields: type `t`, name `n`)
> Regex parser
```regex
t(IPv[46])\nn(\*|\[.+?]|[^:]+):(\d+)
```
> Captured groups
- group 1: IPv4 / IPv6 (used to disambiguate `*`)
- group 2: host (`*`, `[::1]`, `127.0.0.1`, ...)
- group 3: port
</details>
<details>
<summary><code>all_interfaces()</code> (<code>ifconfig</code> + regex)</summary>
> Command used
```bash
ifconfig -au
```
> Regex parsing
Interface blocks:
```regex
(?m)^(?P<iface>\S+):(?P<block>[^\n]*(?:\n[ \t]+[^\n]*)*)
```
IPv4 extraction within each block:
```regex
inet (\S+)
```
</details>
### Notes
The only remaining platform skips on macOS are the USDT/BPF tracing tests (`interface_usdt_*.py`).
ACKs for top commit:
Sjors:
ACK 1950da94fc
achow101:
ACK 1950da94fc
willcl-ark:
tACK 1950da94fc
Tree-SHA512: 4cecc88852623f3fe3a7dccceb0e71932824c1ed7f1d4ab89b953ff6b7991afbd0b016c819c17e966bed53082dd623a832752b8847711861009cd5ffc4677367
1aa78cdab6 clusterlin: adopt STL ranges algorithms (refactor) (Pieter Wuille)
747da25360 feefrac: drop comparison and operator{<<,>>} for sorted wrappers (Pieter Wuille)
Pull request description:
Instead of having an unintuitive but strong implicit sort order on `FeeFrac` (first increasing feerate, then decreasing size), and separate overloaded `operator<<` and `operator>>` that implement a weak ordering that only looks at feerate, replace these with explicit wrapper classes which make the behavior more explicit (`ByRatio` and `ByRatioNegSize`).
This allows for things like `ByRatio{a} <= ByRatio{b}`, instead of the earlier `!(a >> b)`. It also supports usage inside `std::min`/`std::max`/`std::less`, and `std::greater`, so one can use:
* `std::max<ByRatioNegSize<FeeFrac>>(a, b)` to get the highest-feerate `FeeFrac`, tie-breaking by smallest size.
* `std::ranges::sort(v, std::greater<ByRatioNegSize<FeeFrac>>{});` to sort a vector that way.
Suggested in https://github.com/bitcoin/bitcoin/pull/34257#discussion_r2780475893.
ACKs for top commit:
achow101:
ACK 1aa78cdab6
sedited:
ACK 1aa78cdab6
ajtowns:
ACK 1aa78cdab6
Tree-SHA512: d76657b15f6d745e5ca01c67fd5b101fdc418e6301646d14e575b6564bfa2fe0eb40a95a7ff95a4420624ef6b67224d35e4713aa5bbc0d293e08fe44c0cc6db0
This change includes `s/RecursiveMutex/Mutex/` and a pile of
annotations to keep the compiler happy after the type change.
Partially resolves: https://github.com/bitcoin/bitcoin/issues/19303
The only recursive usage of `CConnman::m_nodes_mutex` is from
`PeerManagerImpl::MaybeSetPeerAsAnnouncingHeaderAndIDs()` which uses
nested calls to `CConnman::ForNode()` to trim the size of
`lNodesAnnouncingHeaderAndIDs` to `<= 3`. This need not be nested, so
take it out.
Before:
```
fornode(newnode)
if (size >= 3)
fornode(front) handle removal of front
pop front
push back newnode
```
After:
```
fornode(newnode)
push back newnode
if (size > 3)
fornode(front) handle removal of front
pop front
```
`lNodesAnnouncingHeaderAndIDs` is protected by `cs_main` which is locked
during the entire operation.
08925d5ee7 test: add coverage for loading a wallet in a non-writable directory (furszy)
0218966c0d test: add coverage for wallet creation in non-writable directory (furszy)
bc0090f1d6 wallet: handle non-writable db directories (furszy)
Pull request description:
Make wallet creation and load fail with a clear error when the db directory isn’t writable.
#### 1) For Wallet Creation
Before: creating a wallet would return a generic error:
"SQLiteDatabase: Failed to open database: unable to open database file"
After: creating a wallet returns:
"SQLiteDatabase: Failed to open database in directory <dir_path>: directory is not writable"
#### 2) For Wallet Loading
We currently allow loading wallets located on non-writable directories. This is problematic
because the node crashes on any subsequent write; generating a block is enough to trigger it.
Can be verified just by running the following test on master: 85fa4e2910
Also, to check directory writability, this creates a tmp file rather than relying on the
`permissions()` functions, since perms bits alone may not reliably reflect actual writability
in some systems.
Testing Note:
Pushed the tests in separate commits so they can be cherry-picked on master for comparison.
ACKs for top commit:
rkrux:
re-ACK 08925d5ee7
achow101:
ACK 08925d5ee7
seduless:
Tested ACK 08925d5ee7
Tree-SHA512: e480eab329a1d595fe0b191e83c97956e3ff1d1e335ada8ac6fe72bc4b2bb9b13b0d49db0254d34ad75f816db06d9cd0c21d3063d7d8ee6687a7ea2324c36288
DuplicateMockDatabase is no longer used. Furthermore, as SQLite gets
used more as a database and less as a key value store, this function
gets more complicated and more bug prone. As the benchmarks now run
equivalently quickly with a real database, retaining this duplication
function is no longer necessary.
Instead of making a mock database and duplicating it for the benchmark,
use a real database. Also use setup() to avoid measuring the overhead in
the benchmark.
WalletMigration needs a new wallet with legacy records for each run of
the benchmark. This can be done in setup() rather than duplicating the
records of an initial wallet.
WalletEncrypt needs an unencrypted wallet in order for the benchmark to
encrypt a wallet. This was previously achieved by duplicating the
contents of an initial wallet for each run of the benchmark. We can
instead use setup() to unload the previously loaded wallet and then
create a new wallet with unencrypted keys.
WalletBalance benchmarks the balance computation function and should
exclude the setup step of (optionally) marking caches as dirty. Instead,
that is moved into setup().
The WalletCreate benchmark should only be for creating a wallet and
exclude the unloading of the newly created wallet. Instead, unloading
can be done in setup() and after the benchmark completes.
0bc9d354df multi_index: fix compilation failure with boost >= 1.91 (Cory Fields)
Pull request description:
This effectively reverts a3cb309e7c from PR #30194.
That PR reduced the `multi_index` type signatures as recommended upstream, but this is no longer supported as of boost 1.91 because it is no longer necessary. 1.91 drops support for the pre-c++11 work-arounds that bloated the type signatures to begin with.
The upstream `BOOST_MULTI_INDEX_ENABLE_MPL_SUPPORT` define is meant to provide compatibility with removed features, but it does not work for this case. Using `indexed_by` directly when defining the `multi_index` (as opposed to inheriting from it) works with all versions, and avoids the use of the back-compat define.
This is a slight regression when building against boost < 1.91 because the bloated type signatures are reintroduced in that case, but it's not significant enough to go to the trouble of introducing version detection and ifdefs.
ACKs for top commit:
maflcko:
review ACK 0bc9d354df🎶
hebasto:
ACK 0bc9d354df.
w0xlt:
ACK 0bc9d354df
Tree-SHA512: 883ee998efd16d944628653ca204e3d2acaf2554b2eced40556143a66d6072a3625861d961d1a4a194a7b8d4d448562581e5d11a09380754a5635a871d2a0aa1
Wallet names that are also paths that contain . and .. are unintuitive
and can result in unexpected behavior, particularly in migration.
Therefore we should disallow users from specifying wallet names that
contain . and .. as path elements.
eab72d14d7 refactor: use SignOptions for MutableTransactionSignatureCreator (Sjors Provoost)
5ed41752c5 refactor: use SignOptions for SignTransaction (Sjors Provoost)
dc4a5d1270 refactor: use PSBTFillOptions for filling and signing (Sjors Provoost)
Pull request description:
Replace the `sign`, `finalize` , `bip32derivs` and `sighash_type` arguments that are passed to `FillPSBT()` and `SignPSBTInput()` with a `PSBTFillOptions` struct.
```cpp
struct PSBTFillOptions {
bool sign{true};
std::optional<int> sighash_type{std::nullopt};
bool finalize{true};
bool bip32_derivs{true};
};
```
Additionally this PR introduces:
```cpp
struct SignOptions {
int sighash_type{SIGHASH_DEFAULT};
};
```
This is used by `SignTransaction` and the `MutableTransactionSignatureCreator` constructor.
These changes make it easier to add additional options later without large code churn, such as `avoid_script_path` proposed in #32857. It also makes the use of default boolean options safer compared to positional arguments that can easily get mixed up.
ACKs for top commit:
w0xlt:
reACK eab72d14d7
optout21:
ACK eab72d14d7
sedited:
ACK eab72d14d7
Tree-SHA512: 097e3d042e794c9f47d03e1aafad184a4525aa765d274f6497122d4f41603e902191df6fbf9ce846dbcd7372a159b67e2234da7341ec6a6776be5685e3e6e6ff
This effectively reverts a3cb309e7c from PR #30194.
That PR reduced the multi_index type signatures as recommended upstream, but
this is no longer supported as of boost 1.91 because it is no longer necessary.
1.91 drops support for the pre-c++11 work-arounds that bloated the type
signatures to begin with.
The upstream `BOOST_MULTI_INDEX_ENABLE_MPL_SUPPORT` define is meant to provide
compatibility with removed features, but it does not work for this case. Using
`indexed_by` directly when defining the `multi_index` (as opposed to inheriting
from it) works with all versions, and avoids the use of the back-compat define.
This is a slight regression when building against boost < 1.91 because the
bloated type signatures are reintroduced in that case, but it's not significant
enough to go to the trouble of introducing version detection and ifdefs.
Replace the sign, finalize , bip32derivs and sighash_type arguments which
are passed to FillPSBT() and SignPSBTInput() with a PSBTFillOptions struct.
This makes it easier to add additional options later without large code
churn, such as avoid_script_path proposed in #32857. It also makes the
use of default boolean options safer compared to positional arguments
that can easily get mixed up.
8deed0df06 doc: add release notes for PR 34911 (rkrux)
1a85ca1dff rpc, mempool: rpcdeprecate `bip125-replaceable` key in mempool RPCs reponses (rkrux)
f89d18c3b1 rpc, mempool: rpcdeprecate `fullrbf` key in getmempoolinfo RPC response (rkrux)
Pull request description:
mempoolfullrbf=1 behaviour has been the default since v28 and the argument has
been removed since v29 subsequently.
The getmempoolinfo RPC need not return the deprecated `fullrbf` key in the
response anymore unless the user requests it via -deprecatedrpc=fullrbf node
argument.
Also, remove the `bip125-replaceable` key from the mempool RPCs
responses (because it, too, has been deprecated since v29) unless the user
requests it via -deprecatedrpc=bip125 node argument.
ACKs for top commit:
optout21:
ACK 8deed0df06
sedited:
ACK 8deed0df06
instagibbs:
ACK 8deed0df06
Tree-SHA512: dfbbdf04ae25dafe81758bd85715af80d736e21bb913ab09ff8fd8225587b7a1bf1dd6b0f7ea05135504e11d43371d98e5f5fc6e3717d581e870e8c2bf97811b
a1e534bbf0 doc: clarify clang-tidy in developer notes (rkrux)
Pull request description:
Recently, I ran clang-tidy in my system and had to figure out setting it up
correcty on my system (macOS) & understanding its output while tweaking
few arguments that made the output easier to parse.
This patch documents those in the developer notes.
ACKs for top commit:
sedited:
ACK a1e534bbf0
Tree-SHA512: e95cd44cdad9a9011c18136ec7f1db4e73f78f11c517f94c14cf128beaaea150881868c27e7c5c710c9cab94d285d61dc5e5278f347c229f5df495cd49c532ab
ef21e29298 doc: update llvm based coverage example (Max Edwards)
Pull request description:
The old example was working on my Mac but when run on Linux I was finding that I had missing coverage information.
On some systems the old example of using xargs to pass list of profraw files can exceed xargs's argument limit resulting in multiple executions of llvm-profdata merge command which overwrites it's output file losing coverage information. This PR switches the example to using a file with a list of filenames.
There are other ways to solve this like using bash's globbing or a one liner with temporary file but I felt like this was the most simple, robust and easy to understand.
ACKs for top commit:
vasild:
ACK ef21e29298
sedited:
ACK ef21e29298
Tree-SHA512: 2726d9b38088c1c0b6f04d4622bdc4ea28e16b13a962a3afc842798aa5e0eee4682752c73707c5580a43bdb9d0d84d542afd357f166e4dbb7f6beb7afc0522c8
14547eb489 kernel: guard btck::Handle move-assignment against self-move (Thomas)
Pull request description:
The move-assignment operator for `btck::Handle<>` in `src/kernel/bitcoinkernel_wrapper.h` unconditionally called `DestroyFunc(m_ptr)` before reading the source pointer. On a self-move (`h = std::move(h)`), this destroys the held resource and then restores the now-dangling pointer via `std::exchange(other.m_ptr, nullptr)` (since `&other == this`), which leaves `m_ptr` pointing at freed memory. The destructor then calls `DestroyFunc` again on it, resulting in a double-free.
Trace of `h = std::move(h)` with the old code, where `h.m_ptr == P`:
1. `DestroyFunc(m_ptr)` -> `delete P`. `this->m_ptr` still literally stores the now-dangling value `P`.
2. `std::exchange(other.m_ptr, nullptr)` — because `&other == this`, this reads the dangling `P` back, writes `nullptr` to `m_ptr`, and returns `P`.
3. `m_ptr = P` restores the dangling pointer.
4. `~Handle()` later runs `DestroyFunc(P)` -> double free, UB.
The copy-assignment operator already guards against self-assignment with `if (this != &other)`; the move variant should be symmetric. The standard library requires moved-from objects to be in a valid (at minimum, safely destructible) state, which the previous implementation violated when source and destination alias.
`Handle<>` is the base class of 16 public types in the kernel C++ API wrapper (`Transaction`, `Block`, `BlockHeader`, `ChainParams`, `Context`, `Coin`, `BlockValidationState`, `ScriptPubkey`, `TransactionOutput`, `Txid`, `OutPoint`, `TransactionInput`, `PrecomputedTransactionData`, `BlockHash`, `BlockSpentOutputs`, `TransactionSpentOutputs`), so self-move can arise from generic algorithms operating on containers of these types (`std::sort`, `std::remove`, erase-remove idioms, etc.).
Fix: mirror the copy-assignment pattern by guarding the move-assignment body with `if (this != &other)`, making a self-move a no-op.
Also extend `CheckHandle` in `src/test/kernel/test_kernel.cpp` to exercise self-move-assignment for every `Handle`-derived type, checking that the stored pointer and the serialized bytes (where applicable) are unchanged.
ACKs for top commit:
sedited:
Thanks, ACK 14547eb489
alexanderwiederin:
ACK 14547eb489
yuvicc:
lgtm ACK 14547eb489
Tree-SHA512: 334f5ad3045d5f9b06cc0dd096bc911a992773c59cc469765c2082975a9f7a90f2349b9ad94b4b5127290de1fab2f5424a621384c1b4bb9152de99f5da8ed6aa
1) For wallet load, this fixes a crash.
We currently allow loading wallets located on non-writable directories.
This is problematic because the node crashes on any subsequent write.
E.g. generating a block is enough to trigger it.
2) For wallet creation, this improves the returned error msg.
Before: creating a wallet would return a generic error:
"SQLiteDatabase: Failed to open database: unable to open database file"
After: creating a wallet returns:
"SQLiteDatabase: Failed to open database in directory <dir_path>: directory
is not writable"
fa204100e1 streams: Remove confusing DataStream::in_avail() (MarcoFalke)
fa5ab0220e move-only: Extract ProcessPong() helper (MarcoFalke)
Pull request description:
The `in_avail` alias is confusing (see commit message), so remove it. Also extract the `ProcessPong()` helper, as the only place that called `in_avail`.
ACKs for top commit:
l0rinc:
code review ACK fa204100e1
sedited:
ACK fa204100e1
Tree-SHA512: b5a069abb471c42d39e91a3c55fc7f18fb9ccf8591408b3dd43210d0287785369b6a5d6b59b847d85247a494f208dbbe4a7e88b24caf9b3e6c8fea2157cd0ee0
2a90b6132a Add release notes for exportasmap (Fabian Jahr)
8cb2d926b4 rpc: Add exportasmap RPC (Fabian Jahr)
Pull request description:
This depends on the embedded data PR itself (#28792), until merge this will remain in draft status. All commit but the last one are from there.
There has been interest in exporting the embedded data back into a file. This is implemented here with a simple `exportasmap` RPC which provides this functionality. The exported file can be used to verify the data integrity or statistical analysis using e.g. `contrib/asmap-tool.py`.
ACKs for top commit:
achow101:
ACK 2a90b6132a
sedited:
ACK 2a90b6132a
seduless:
Tested ACK 2a90b6132a
Tree-SHA512: c8453078efe916ed95bff0695aabd9123d805970e037fad5e9317f4d43e2a4b21970030265bc82b00f3a222ee3db11346eef1fb6fc9393429b9bc6449f1830f1
fa43da21f1 refactor: Run ShouldWarnOversizedDbCache calculation in u64 (MarcoFalke)
fa5801762e util: Return uint64_t from _MiB and _GiB operators (MarcoFalke)
fa9ddb01c9 test: Use MiB operator directly in cuckoocache_tests (MarcoFalke)
Pull request description:
Currently, the `_MiB` literal operator returns a value of type `size_t`. This is brittle and confusing:
* It is inherently impossible to represent larger values, like storage device usage.
* Similarly, it is not possible to even type an upper cap on the memory, see the failure in https://github.com/bitcoin/bitcoin/pull/34692#issuecomment-3973281952
* Using `size_t` isn't required here. The function is evaluated at compile time, and even 32-bit compilers can evaluate an `uint64_t` at compile time.
* Using `size_t` here encourages it to be used in more places, which will likely lead to more bugs and CVEs, such as https://github.com/bitcoin/bitcoin/pull/34109, https://github.com/bitcoin/bitcoin/pull/33724, etc.
Fix all issues, by:
* Marking the operator `consteval`, to ensure it is really only called at compile-time.
* Returning an `uint64_t` value.
* Using it in the place where it was previously not possible.
Review note:
This should have no downside, because the C++11 narrowing checks continue to work as expected. For example, typing `uint8_t{1_MiB};` will continue to fail with the correct error message `error: constant expression evaluates to 1048576 which cannot be narrowed to type 'uint8_t' (aka 'unsigned char') [-Wc++11-narrowing]` (like it does on current master).
ACKs for top commit:
l0rinc:
ACK fa43da21f1
achow101:
ACK fa43da21f1
hebasto:
ACK fa43da21f1, I have reviewed the code and it looks OK.
hodlinator:
ACK fa43da21f1
Tree-SHA512: e19f6bd18c519c2a56d44fbc9e65b02e630bc71170f02a8a8955bf5c9bb2924b55ec2f810de2bbc961162aa668b7383c9fe9b5261fa1de7d9e8413003c32fa87
On some systems the old example of using xargs to pass list of profraw
files can exceed xargs's argument limit resulting in multiple executions
of llvm-profdata merge command which overwrites it's output file losing
coverage information. Switch to using a file with a list of filenames.
Recently, I ran clang-tidy in my system and had to figure out setting it up
correcty on my system (macOS) & understanding its output while tweaking
few arguments that made the output easier to parse.
This patch documents those in the developer notes.
The alias of the size() method is confusing, because:
* It claims to be part of the Bitcoin Core stream subset (streams
interface), but this is not used by any other stream interface. Mostly
the `write(std::span)` and `read(std::span)` define the stream
interface.
* It casts the size_t to i32, but the only place that calls the function
casts that back to size_t.
* Providing this alias for size() without a proper reason is confusing.
Fix all issues by removing it and using the size() method.
