113a422822 wallet: Add m_cached_from_me to cache "from me" status (Ava Chow)
609d265ebc test: Add a test for anchor outputs in the wallet (Ava Chow)
c40dc822d7 wallet: Throw an error in sendall if the tx size cannot be calculated (Ava Chow)
39a7dbdd27 wallet: Determine IsFromMe by checking for TXOs of inputs (Ava Chow)
e76c2f7a41 test: Test wallet 'from me' status change (Ava Chow)
Pull request description:
One of the ways that the wallet would determine if a transaction was sent from the wallet was by checking if the total amount being spent by a transaction from outputs known to the wallet was greater than 0. This has worked fine until recently since there was no reason for 0-value outputs to be created. However, with ephemeral dust and P2A, it is possible to create standard 0-value outputs, and the wallet was not correctly identifying the spends of such outputs. This PR updates `IsFromMe` to only check whether the wallet knows any of the inputs, rather than checking the debit amount of a transaction.
Additionally, a new functional test is added to test for this case, as well as a few other anchor output related scenarios. This also revealed a bug in `sendall` which would cause an assertion error when trying to spend all of the outputs in a wallet that has anchor outputs.
Fixes#33265
ACKs for top commit:
rkrux:
lgtm ACK 113a422822
enirox001:
Tested ACK 113a422. Ran the full functional test suite including `wallet_anchor.py`; all tests passed. Fix for 0 value anchor detection and sendall size errors looks good. LGTM.
furszy:
ACK 113a422822
Tree-SHA512: df2ce4b258d1875ad0b4f27a5b9b4437137a5889a7d5ed7fbca65f904615e9572d232a8b8d070760f75ac168c1a49b7981f6b5052308575866dc610d191ca964
93a29ff283 trace: Workaround GCC bug compiling with old systemtap (Luke Dashjr)
Pull request description:
ACKs for top commit:
0xB10C:
lgtm ACK 93a29ff283 - I did not test this.
Tree-SHA512: 9ce9ed8b7733af721134462073a3417e52d67e9e9853eebbddfa795842b381de98e28756ebfa6652536cbfdd08181142eccd198f4dc00a57d8748801b362b4b7
653a9849d5 common: Make arith_uint256 trivially copyable (Fabian Jahr)
Pull request description:
Makes `arith_uint256`/`base_uint` trivially copyable by removing the custom copy constructor and copy assignment operators. Removing of the custom code should not result in a change of behavior since `base_uint` contains a simple array of `uint32_t` and compiler generated versions of the code could be better optimized.
This was suggested by maflcko here: https://github.com/bitcoin/bitcoin/pull/30469#pullrequestreview-3186533494
ACKs for top commit:
Raimo33:
ACK 653a9849d5
l0rinc:
ACK 653a9849d5
achow101:
ACK 653a9849d5
hodlinator:
re-ACK 653a9849d5
Tree-SHA512: 38db5220a2cf773c0c5fb5591671e329b6b87458d972db4f5f3f98c025ec329a8c39b32b5bc24ef8b50b1002b43bb248d8b35aa1c9a56c68c6bbd1d470485bd7
75d9b72475 kernel: make blockTip index const (stickies-v)
Pull request description:
Notification interface subscribers need to view, but not mutate, the index.
This change allows improving the #30595 kernel interface, see e.g. `BlockTreeEntry` where [currently](https://github.com/bitcoin/bitcoin/pull/30595/files#diff-4d05cd02fdce641be603f0f9abcecfeaf76944285d4539ba4bbc40337fa9bbc2R617) a `View` is constructed from a non-const pointer, whereas really this should be a `const btck_BlockTreeEntry* entry`.
ACKs for top commit:
achow101:
ACK 75d9b72475
TheCharlatan:
ACK 75d9b72475
l0rinc:
Code review ACK 75d9b72475
yuvicc:
Code review ACK 75d9b72475
Tree-SHA512: 6151374a040cead36490c5fa5ce9dc4d93499a02110f444c50bd90f9095912747bc5b2fd7294815e6794c96a6843f43eb0507706d41d7296af96071b5f704ff4
d45f3717d2 txgraph: use enum Level instead of bool main_only (Pieter Wuille)
Pull request description:
Part of #30289. Inspired by https://github.com/bitcoin/bitcoin/pull/28676#discussion_r2331387778.
Since there has been more than one case in the development of #28676 of calling a `TxGraph` function without correctly setting the `bool main_only` argument that many of its interface functions have, make these mandatory and explicit, using an `enum class Level`:
```c++
enum class Level {
TOP, //!< Refers to staging if it exists, main otherwise.
MAIN //!< Always refers to the main graph, whether staging is present or not.
};
```
ACKs for top commit:
instagibbs:
ACK d45f3717d2
vasild:
ACK d45f3717d2
glozow:
code review ACK d45f3717d2
Tree-SHA512: d1c4b37e8ab3ec91b414df8970cb47aa080803f68da5881c8e1cbdc6939dea7851e0f715192cf3edd44b7f328cd6b678474d41f9cd9da8cb68f6c5fd78cb71b1
Replacing the custom code with default behavior should not result in a change of behavior since base_uint contains a simple array of uint32_t and compiler generated versions of the code could be better optimized.
Co-authored-by: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz>
6a371b70c8 gui: Avoid pathological QT text/markdown behavior... (David Gumberg)
Pull request description:
...during text selection by only setting plaintext mime data.
Fixes the OOM described in #887.
The issue is related to the construction of the [`text/markdown`](b617d11765/src/widgets/widgets/qwidgettextcontrol.cpp (L3539)) MIME data for the selection. Using the `heaptrack` utility, I observed that nearly all of the allocations when reproducing happen in [`QTextMarkdownWriter::writeFrame`](b617d11765/src/gui/text/qtextmarkdownwriter.cpp (L95)). I am not 100% sure what is causing this issue in QT's conversion of our HTML to markdown; I have tried changing the [HTML tags](689a321976/src/qt/rpcconsole.cpp (L916-L924)) (e.g. using `<p></p`> and `<ul><li></li></ul>` in place of tables) used in our `rpcconsole` messages, but the issue recurs.
The solution applied here is to override `createMimeDataFromSelection()` to avoid construction of the (likely never-used anyways) `text/markdown` mime data, and only set plaintext mime data in the clipboard.
ACKs for top commit:
hebasto:
ACK 6a371b70c8.
Tree-SHA512: 3edc4da47e6dbe939f27664d2265376938eed4f83ded3706e4b73677eac5c9a4ba8819f241428b45a08e8834982ee7759ee096afd090586db3b523d0ccbbbf73
4d4789dffa net: Prevent node from binding to the same CService (woltx)
Pull request description:
Currently, if the node inadvertently starts with repeated `-bind` options (e.g. `./build/bin/bitcoind -listen -bind=0.0.0.0 -bind=0.0.0.0`), the user will receive a misleading message followed by the node shutdown:
```
[net:error] Unable to bind to 0.0.0.0:8333 on this computer. Bitcoin Core is probably already running.
[error] Unable to bind to 0.0.0.0:8333 on this computer. Bitcoin Core is probably already running.
```
And the user might spend some time looking for a `bitcoind` process or what application is using port 8333, when what happens is that Bitcoin Core successfully connected to port 8333 and then tries again, generating this fatal error.
This PR proposes that repeated `-bind` options have no effect.
ACKs for top commit:
l0rinc:
ACK 4d4789dffa
yuvicc:
re-ACK 4d4789dffa
sipa:
utACK 4d4789dffa
achow101:
ACK 4d4789dffa
vasild:
ACK 4d4789dffa
naiyoma:
Tested ACK 4d4789dffa
Tree-SHA512: f1042c00417da16550403cfcb75cb8b12740e67cf92a1d8e3c007ae81fcf741907088a633129ce12a6a48ad07fc9f320602792cafed73ec33f6306cd854514b4
d3c5e47391 wallet, refactor: Remove Legacy check and error (pablomartin4btc)
30c6f64eed test: Remove unnecessary LoadWallet() calls (pablomartin4btc)
Pull request description:
Remove dead code due to legacy wallet removal.
Leftovers from previous #32481.
---
**Note**:
While attempting to remove the legacy check in `CWallet::UpgradeDescriptorCache()` (which is called from `DBErrors WalletBatch::LoadWallet(CWallet* pwallet))`, I once again ran into the fact that `LoadWallet()` is used in two distinct scenarios — something I was already aware of:
- Wallet creation – the upgrade is ignored here because no wallet flags are yet set; attempting to set a flag (ie `WALLET_FLAG_LAST_HARDENED_XPUB_CACHED` at the end of the upgrade function, if the legacy check is removed) would produce a failure (`DBErrors CWallet::LoadWallet()` -> `Assert(m_wallet_flags == 0)`).
- Wallet loading – the upgrade proceeds correctly and the flag `WALLET_FLAG_LAST_HARDENED_XPUB_CACHED` is set.
While revisiting this, I also noticed that some `LoadWallet()` calls in the wallet tests are unnecessary and I've removed them in the first commit.
The following change in `UpgradeDescriptorCache()` could be done in PR #32636 as part of the separation between wallet loading and creation responsibilities.
```diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp
void CWallet::UpgradeDescriptorCache()
{
+ // Only descriptor wallets can upgrade descriptor cache
+ Assert(IsWalletFlagSet(WALLET_FLAG_DESCRIPTORS));
+
- if (!IsWalletFlagSet(WALLET_FLAG_DESCRIPTORS) || IsLocked() || IsWalletFlagSet(WALLET_FLAG_LAST_HARDENED_XPUB_CACHED)) {
+ if (IsLocked() || IsWalletFlagSet(WALLET_FLAG_LAST_HARDENED_XPUB_CACHED)) {
return;
}
```
ACKs for top commit:
davidgumberg:
crACK d3c5e47391
achow101:
ACK d3c5e47391
l0rinc:
code review ACK d3c5e47391
Tree-SHA512: ead37cf4061dfce59feb41ac50e807e6790e1a5e6b358e3b9c13e63d61a9cb82317a2e596cecb543f62f88a4338171788b651452425c1f40b5c1bec7fe78339e
790b440197 Fix benchmark CSV output (Hennadii Stepanov)
Pull request description:
The `SHA256AutoDetect` return output is used, among other use cases, to name benchmarks. Using a comma breaks the `bench_bitcoin` CSV output.
This PR replaces the comma with a semicolon, which fixes https://github.com/bitcoin/bitcoin/issues/33331.
ACKs for top commit:
Raimo33:
Code Review ACK 790b440197
l0rinc:
Code review ACK 790b440197
janb84:
code review ACK 790b440197
Tree-SHA512: 096bfa29a0639a4d97d510a3e2a15f071f384148c3035e4d0fc525794682e499c45a0d0c95728d5c78010098393b2c486a7fa9c21c1e2fbb600dea7c5638a55f
8b62647680 test: send duplicate blocktxn message in p2p_compactblocks.py (Eugene Siegel)
5e585a0fc4 net: check for empty header before calling FillBlock (Eugene Siegel)
Pull request description:
This avoids an Assume crash if multiple blocktxn messages are received. The first call to `FillBlock` would make the header empty via `SetNull` and the call right before the second `FillBlock` would crash [here](689a321976/src/net_processing.cpp (L3333)) since `LookupBlockIndex` won't find anything. Fix that by checking for an empty header before the Assume.
ACKs for top commit:
instagibbs:
reACK 8b62647680
fjahr:
tACK 8b62647680
achow101:
ACK 8b62647680
mzumsande:
Code Review ACK 8b62647680
Tree-SHA512: d43a6f652161d4f7e6137f207a3e95259fc51509279d20347b1698c91179c39c8fcb75d2668b13a6b220f478a03578573208a415804be1d8843acb057fa1a73a
c767974811 clang-tidy: Fix critical warnings (Fabian Jahr)
54dc34ec22 index: Remove unused coinstatsindex recovery code (Fabian Jahr)
37c4fba1f4 index: Check BIP30 blocks when rewinding Coinstatsindex (Fabian Jahr)
51df9de8e5 doc: Add release note for 30469 (Fabian Jahr)
bb8d673183 test: Add coinstatsindex compatibility test (Fabian Jahr)
b2e8b64ddc index, refactor: Append blocks to coinstatsindex without db read (Fabian Jahr)
431a076ae6 index: Fix coinstatsindex overflow issue (Fabian Jahr)
84e813a02b index, refactor: DRY coinbase check (Fabian Jahr)
fab842b324 index, refactor: Rename ReverseBlock to RevertBlock (Fabian Jahr)
Pull request description:
Closes https://github.com/bitcoin/bitcoin/issues/26362
This continues the work that was started with #26426. It fixes the overflow issue by switching the tracked values that are in danger of overflowing from `CAmount` to `arith_uint256`.
The current approach opts for a simple solution to ensure compatibility with datadirs including the previous version of the index: The new version of the index goes into a separate location in the datadir (`index/coinstatsindex/` rather than `index/coinstats/` before, the new naming is more consistent with the naming of the other indexes). There is no explicit concept of versioning of the index which earlier versions of this PR had. Having the two different versions of the index in separate places allows for downgrading of the node without having to rebuild the index. However, there will be a warning printed in the logs if the new code (v30) detects the old index still being present. A future version could delete a left-over legacy index automatically.
The PR also includes several minor improvements but most notably it lets new entries be calculated and stored without needing to read any DB records.
ACKs for top commit:
achow101:
ACK c767974811
TheCharlatan:
ACK c767974811
mzumsande:
Tested / Code Review ACK c767974811
Tree-SHA512: 3fa4a19dd1a01c1b01390247bc9daa6871eece7c1899eac976e0cc21ede09c79c65f758d14daafc46a43c4ddd7055c85fb28ff03029132d48936b248639c6ab9
Previously in debug builds, this would cause an Assume crash if
FillBlock had been called previously. This could happen when multiple
blocktxn messages were received.
Co-Authored-By: Greg Sanders <gsanders87@gmail.com>
188de70c86 net: Add interrupt to pcp retry loop (TheCharlatan)
Pull request description:
Without this interrupt bitcoind takes a long time to exit if requested to do so after a failed pcp lookup on startup.
ACKs for top commit:
achow101:
ACK 188de70c86
fjahr:
utACK 188de70c86
hodlinator:
utACK 188de70c86
Tree-SHA512: 426dabd10ac0ef5de246c83d281ba70957e4032d251054aa6028b4d7ce4e35cd35ac70e67dc07bd418673bcdd2f4457b76f174ac5e7d0dd3caa05de5da952dac
589b65f06c clang-tidy: Disable `UndefinedBinaryOperatorResult` check in `src/ipc` (Hennadii Stepanov)
Pull request description:
The warnings are false positive and have been fixed upstream. See: https://github.com/capnproto/capnproto/pull/2334.
This PR:
1. Disables the `UndefinedBinaryOperatorResult` clang-tidy check for source files generated by the `mpgen` tool.
2. Is an alternative to the draft https://github.com/bitcoin/bitcoin/pull/33281.
3. Fixes https://github.com/bitcoin/bitcoin/issues/33256.
ACKs for top commit:
Sjors:
ACK 589b65f06c
fjahr:
ACK 589b65f06c
achow101:
ACK 589b65f06c
ryanofsky:
Code review ACK 589b65f06c. Thanks for the fix!
Tree-SHA512: 6d376a82641a5b85d4dd1fa164fdcbd8e15f1262e7d4f582f4d9959031d35852e28ff1b8268336e39ba6779fdd10ecdb986af42407d0545f4217f41d64556272
The `SHA256AutoDetect` return output is used, among other use cases, to
name benchmarks. Using a comma breaks the CSV output.
This change replaces the comma with a semicolon, which fixes the issue.
The std::move in coinstatsindex was not necessary since it was passed as a const reference argument.
The other change in the utxo supply fuzz test changes a line that seems to have triggered a false alarm.
The coinstatsindex currently looks for block data at a hash key if the prev block in CustomAppend is different than expected. This is not needed since base index should always prevent us ending up in this scenario since it should rewind the index before calling CustomAppend in this case. But even if we run into this and our belt-and-suspenders code is getting hit, the index could not recover properly from the hash key index data so it can be removed without any real impact.
This is practically irrelevant due to the unlikeliness of a re-org
reaching so deep that it would drop the BIP30 blocks from the chain
(91842 and 91880). However this serves as documentation and ensures that
the functions RevertBlock and CustomAppend are consistent.
The index originally stored cumulative values in a CAmount type but this allowed for
potential overflow issues which were observed on Signet. Fix this by
storing the values that are in danger of overflowing in a arith_uint256.
Also turns an unnecessary copy into a reference in RevertBlock and
CustomAppend and gets
rid of the explicit total unspendable tracking which can be calculated
by adding the four categories of unspendables together.
Currently, if the user inadvertently starts the node with duplicate bind options,
such as `-bind=0.0.0.0 -bind=0.0.0.0`, it will cause a fatal error with the
misleading message "Bitcoin Core is probably already running".
This commit adds early validation to detect duplicate bindings across all binding
configurations (-bind, -whitebind, and onion bindings) before attempting to bind.
When duplicates are detected, the node terminates with a clear, specific error
message: "Duplicate binding configuration for address <addr>. Please check your
-bind, -bind=...=onion and -whitebind settings."
The validation catches duplicates both within the same option type (e.g.,
`-bind=X -bind=X`) and across different types (e.g., `-bind=X -whitebind=Y@X`),
helping users identify and fix configuration mistakes.
The warnings are false positive and have been fixed upstream.
See: https://github.com/capnproto/capnproto/pull/2334.
This change disables the `UndefinedBinaryOperatorResult` clang-tidy
check for source files generated by the `mpgen` tool.
When the router doesn't support natpmp and PCP, one'd normally expect
the UDP packet to be ignored, and hit a time out. This logs a warning
that is already in the debug category. However, there's also the case in
which sending an UDP packet causes a ICMP response. This is returned to
user space as "connection refused" (despite UDP having no concept of
connections).
Move the warnings from `Send` and `Recv` to debug level too, to reduce
log spam in that case.
Closes#33301.
88db09bafe net: handle multi-part netlink responses (willcl-ark)
42e99ad773 net: skip non-route netlink responses (willcl-ark)
57ce645f05 net: filter for default routes in netlink responses (willcl-ark)
Pull request description:
...for default route in pcp pinholing.
Currently we only make a single recv call, which trucates results from large routing tables, or in the case the kernel may split the message into multiple responses (which may happen with `NLM_F_DUMP`).
We also do not filter on the default route. For IPv6, this led to selecting the first route with an `RTA_GATEWAY` attribute, often a non-default route instead of the actual default. This caused PCP port mapping failures because the wrong gateway was used.
Fix both issues by adding multi-part handling of responses and filter for the default route.
Limit responses to ~ 1MB to prevent any router-based DoS.
ACKs for top commit:
achow101:
ACK 88db09bafe
davidgumberg:
Code Review re-ACK 88db09b
Sjors:
re-utACK 88db09bafe
Tree-SHA512: ea5948edebfad5896a487a61737aa5af99f529fad3cf3da68dced456266948238a7143383847e79a7bb90134e023eb173c25116d8eb80ff57fa4c4a0377ca1ed
Handle multi-part netlink responses to prevent truncated results from
large routing tables.
Previously, we only made a single recv call, which led to incomplete
results when the kernel split the message into multiple responses (which
happens frequently with NLM_F_DUMP).
Also guard against a potential hanging issue where the code would
indefinitely wait for NLMSG_DONE for non-multi-part responses by
detecting the NLM_F_MULTI flag and only continue waiting when necessary.
m_cached_from_me is used to track whether a transaction is "from me", i.e. has
any inputs which belong to the wallet. This is held in memory only in
the same way that a transaction's balances are.
Instead of checking whether the total amount of inputs known by the
wallet is greater than 0, we should be checking for whether the input is
known by the wallet. This enables us to determine whether a transaction
spends an of output with an amount of 0, which is necessary for marking
0-value dust outputs as spent.
2885bd0e1c doc: unify `datacarriersize` warning with release notes (Lőrinc)
Pull request description:
Follow-up to https://github.com/bitcoin/bitcoin/pull/32406
---
The [release notes](a189d63618/doc/release-notes-32406.md (L1)) claim
> [...] marked as deprecated and are expected to be removed in a future release
but the [warning itself](2885bd0e1c/src/init.cpp (L907)) claims
> [...] marked as deprecated. They **will** be removed in a future version.
To be less aggressive (since some have objected against this version online) - and to unify the deprecation warning with the release notes - I have changed the warning to communicate our expectation in a friendlier way.
ACKs for top commit:
cedwies:
ACK 2885bd0
ryanofsky:
Code review ACK 2885bd0e1c. I don't think it is good for the release notes and the runtime warning message to say two different things. I'd also be happy if release notes were updated to match the runtime warning, instead of vice versa. Whatever is more accurate is better.
ajtowns:
ACK 2885bd0e1c
kevkevinpal:
ACK [2885bd0](2885bd0e1c)
achow101:
ACK 2885bd0e1c
janb84:
ACK 2885bd0e1c
Zero-1729:
crACK 2885bd0e1c
jonatack:
ACK 2885bd0e1c
hodlinator:
ACK 2885bd0e1c
w0xlt:
ACK 2885bd0e1c
optout21:
ACK 2885bd0e1c
Tree-SHA512: a9d2a64ab96b3dd7f3a1a29622930054fd5c56e573bc96330f4ef3327dc024b21b3fbc8a698d17aea7c76f57f0c2ccd6403b2df344ae2f69c645ceb8b6fa54a5
