💬 rkrux commented on pull request "wallet: Add `exportwatchonlywallet` RPC to export a watchonly version of a wallet":
(https://github.com/bitcoin/bitcoin/pull/32489#discussion_r2300740989)
In 3a30ce4cecfb25c353ec4e5f178f47668214bb4a "wallet: Add CWallet::ExportWatchOnly"
Because there is no actual use of keys and error here in this flow.
```diff
diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp
index 4bfa11c0bf..95061559c7 100644
--- a/src/wallet/wallet.cpp
+++ b/src/wallet/wallet.cpp
@@ -4562,10 +4562,11 @@ util::Result<std::string> CWallet::ExportWatchOnlyWallet(const fs::path& destina
// Parse the descriptors and add them to the new wallet
...
(https://github.com/bitcoin/bitcoin/pull/32489#discussion_r2300740989)
In 3a30ce4cecfb25c353ec4e5f178f47668214bb4a "wallet: Add CWallet::ExportWatchOnly"
Because there is no actual use of keys and error here in this flow.
```diff
diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp
index 4bfa11c0bf..95061559c7 100644
--- a/src/wallet/wallet.cpp
+++ b/src/wallet/wallet.cpp
@@ -4562,10 +4562,11 @@ util::Result<std::string> CWallet::ExportWatchOnlyWallet(const fs::path& destina
// Parse the descriptors and add them to the new wallet
...
💬 rkrux commented on pull request "wallet: Add `exportwatchonlywallet` RPC to export a watchonly version of a wallet":
(https://github.com/bitcoin/bitcoin/pull/32489#discussion_r2301117939)
Fair point.
I can imagine one of the use cases being that the user creates a offline wallet and immediately exports the watchonly version that's to be imported on a connected node.
(https://github.com/bitcoin/bitcoin/pull/32489#discussion_r2301117939)
Fair point.
I can imagine one of the use cases being that the user creates a offline wallet and immediately exports the watchonly version that's to be imported on a connected node.
✅ tomt1664 closed a pull request: "Bug Fix: invalid address error message and test"
(https://github.com/bitcoin/bitcoin/pull/33257)
(https://github.com/bitcoin/bitcoin/pull/33257)
💬 cedwies commented on pull request "net: Prevent node from binding to the same `CService`":
(https://github.com/bitcoin/bitcoin/pull/33231#discussion_r2301123345)
Nice and clear! One small UX suggestion: since we already know which container the duplicate came from, it could be helpful to mention the categories in the error (e.g. “-bind vs -whitebind”). That said, even without categories, I think this should at least be translatable for consistency with nearby init errors (e.g. `i2psam`). Something like:
```cpp
return InitError(strprintf(
_("Duplicate binding configuration for address %s. "
"Please check your -bind, -whitebind, and onion bindin
...
(https://github.com/bitcoin/bitcoin/pull/33231#discussion_r2301123345)
Nice and clear! One small UX suggestion: since we already know which container the duplicate came from, it could be helpful to mention the categories in the error (e.g. “-bind vs -whitebind”). That said, even without categories, I think this should at least be translatable for consistency with nearby init errors (e.g. `i2psam`). Something like:
```cpp
return InitError(strprintf(
_("Duplicate binding configuration for address %s. "
"Please check your -bind, -whitebind, and onion bindin
...
💬 fanquake commented on pull request "[29.x] backport #33212":
(https://github.com/bitcoin/bitcoin/pull/33251#issuecomment-3224597646)
cc @mzumsande
(https://github.com/bitcoin/bitcoin/pull/33251#issuecomment-3224597646)
cc @mzumsande
💬 instagibbs commented on pull request "Revert compact block cache inefficiencies":
(https://github.com/bitcoin/bitcoin/pull/33253#issuecomment-3224827005)
Benchmarks now match expectation: each commit separately improves the benchmarks :heavy_check_mark:
(https://github.com/bitcoin/bitcoin/pull/33253#issuecomment-3224827005)
Benchmarks now match expectation: each commit separately improves the benchmarks :heavy_check_mark:
💬 brunoerg commented on pull request "fuzz: add target for `DifferenceFormatter` and p2p invariant check":
(https://github.com/bitcoin/bitcoin/pull/33252#discussion_r2301545461)
Stiil needs to update the PR title.
(https://github.com/bitcoin/bitcoin/pull/33252#discussion_r2301545461)
Stiil needs to update the PR title.
📝 fanquake opened a pull request: "ci: use LLVM 21"
(https://github.com/bitcoin/bitcoin/pull/33258)
Use LLVM 21 in the *san & fuzz CIs.
(https://github.com/bitcoin/bitcoin/pull/33258)
Use LLVM 21 in the *san & fuzz CIs.
💬 enirox001 commented on pull request "test: Fix CLI_MAX_ARG_SIZE issues":
(https://github.com/bitcoin/bitcoin/pull/33243#discussion_r2301586217)
That’s reasonable. Since the test uses ASCII 'a' and the sizes are large, encoding won’t change the outcome in practice.
(https://github.com/bitcoin/bitcoin/pull/33243#discussion_r2301586217)
That’s reasonable. Since the test uses ASCII 'a' and the sizes are large, encoding won’t change the outcome in practice.
🤔 enirox001 reviewed a pull request: "test: Fix CLI_MAX_ARG_SIZE issues"
(https://github.com/bitcoin/bitcoin/pull/33243#pullrequestreview-3156543444)
ACK fa96a4a — thanks for addressing the nits and clarifying the test; LGTM.
(https://github.com/bitcoin/bitcoin/pull/33243#pullrequestreview-3156543444)
ACK fa96a4a — thanks for addressing the nits and clarifying the test; LGTM.
✅ w0xlt closed an issue: "`feature_bind_extra.py` test fails in `test_runner` if new nodes are added"
(https://github.com/bitcoin/bitcoin/issues/33250)
(https://github.com/bitcoin/bitcoin/issues/33250)
💬 achow101 commented on pull request "rpc: Handle -named argument parsing where '=' character is used":
(https://github.com/bitcoin/bitcoin/pull/32821#discussion_r2301652903)
My point was more that the criteria appears to be applied inconsistently.
(https://github.com/bitcoin/bitcoin/pull/32821#discussion_r2301652903)
My point was more that the criteria appears to be applied inconsistently.
💬 achow101 commented on pull request "rpc: Handle -named argument parsing where '=' character is used":
(https://github.com/bitcoin/bitcoin/pull/32821#discussion_r2301654129)
I don't think the meaning is different enough to warrant a whole new class.
(https://github.com/bitcoin/bitcoin/pull/32821#discussion_r2301654129)
I don't think the meaning is different enough to warrant a whole new class.
💬 w0xlt commented on issue "`feature_bind_extra.py` test fails in `test_runner` if new nodes are added":
(https://github.com/bitcoin/bitcoin/issues/33250#issuecomment-3225070972)
I haven’t fully reviewed this code yet, but at first glance it seems that if the issue were related to the P2P port, the assert condition in `test/functional/test_framework/util.py` should have failed.
```
def p2p_port(n):
assert n <= MAX_NODES
return PORT_MIN + n + (MAX_NODES * PortSeed.n) % (PORT_RANGE - 1 - MAX_NODES)
```
Anyway, it doesn’t seem like much of a concern, and since the PR doesn’t touch the test further, I’ll close this for now.
(https://github.com/bitcoin/bitcoin/issues/33250#issuecomment-3225070972)
I haven’t fully reviewed this code yet, but at first glance it seems that if the issue were related to the P2P port, the assert condition in `test/functional/test_framework/util.py` should have failed.
```
def p2p_port(n):
assert n <= MAX_NODES
return PORT_MIN + n + (MAX_NODES * PortSeed.n) % (PORT_RANGE - 1 - MAX_NODES)
```
Anyway, it doesn’t seem like much of a concern, and since the PR doesn’t touch the test further, I’ll close this for now.
💬 fanquake commented on issue "Linux download needs installation instructions":
(https://github.com/bitcoin/bitcoin/issues/32097#issuecomment-3225097225)
Yea I'm not sure. For master, the binaries still have a `libxcb-cursor` dependency, that may need to be installed. It's not clear if #33238 will work for `29.x`, so that will still have a `libxcb-xinerama` dependency, that may need to be installed, (same for `28.x`). There's a note about `libxcb-cursor0` in build-unix, but that isn't shipped with release binaries, or likely read by anyone who isn't compiling from source, so it seems like any of this could benefit from additional documentation.
...
(https://github.com/bitcoin/bitcoin/issues/32097#issuecomment-3225097225)
Yea I'm not sure. For master, the binaries still have a `libxcb-cursor` dependency, that may need to be installed. It's not clear if #33238 will work for `29.x`, so that will still have a `libxcb-xinerama` dependency, that may need to be installed, (same for `28.x`). There's a note about `libxcb-cursor0` in build-unix, but that isn't shipped with release binaries, or likely read by anyone who isn't compiling from source, so it seems like any of this could benefit from additional documentation.
...
💬 maflcko commented on issue "Linux download needs installation instructions":
(https://github.com/bitcoin/bitcoin/issues/32097#issuecomment-3225106768)
> There's a note about `libxcb-cursor0` in build-unix, but that isn't shipped with release binaries, or likely read by anyone who isn't compiling from source, so it seems like any of this could benefit from additional documentation.
I see, so it seems a bit related to https://github.com/bitcoin/bitcoin/issues/32565
(https://github.com/bitcoin/bitcoin/issues/32097#issuecomment-3225106768)
> There's a note about `libxcb-cursor0` in build-unix, but that isn't shipped with release binaries, or likely read by anyone who isn't compiling from source, so it seems like any of this could benefit from additional documentation.
I see, so it seems a bit related to https://github.com/bitcoin/bitcoin/issues/32565
💬 ajtowns commented on pull request "Revert compact block cache inefficiencies":
(https://github.com/bitcoin/bitcoin/pull/33253#discussion_r2301680659)
Added an assert and comment.
(https://github.com/bitcoin/bitcoin/pull/33253#discussion_r2301680659)
Added an assert and comment.
💬 maflcko commented on issue "`feature_bind_extra.py` test fails in `test_runner` if new nodes are added":
(https://github.com/bitcoin/bitcoin/issues/33250#issuecomment-3225111710)
The assert is not hit, because you are just setting `port+=2`, not by calling `p2p_port(node_number+N)`.
My recommendation would be to call `p2p_port`, so that the assert is hit in your code.
(https://github.com/bitcoin/bitcoin/issues/33250#issuecomment-3225111710)
The assert is not hit, because you are just setting `port+=2`, not by calling `p2p_port(node_number+N)`.
My recommendation would be to call `p2p_port`, so that the assert is hit in your code.
⚠️ maflcko reopened an issue: "`feature_bind_extra.py` test fails in `test_runner` if new nodes are added"
(https://github.com/bitcoin/bitcoin/issues/33250)
When new nodes are added to the `feature_bind_extra.py` test, it succeeds if run individually, but fails if run via `test_runner` with the error message:
```
TestFramework (ERROR): Unexpected exception
Traceback (most recent call last):
File "/home/node/test/functional/test_framework/test_framework.py", line 191, in main
self.setup()
File "/home/node/test/functional/test_framework/test_framework.py", line 349, in setup
self.setup_network()
File "/home/node/build/test/functional/fe
...
(https://github.com/bitcoin/bitcoin/issues/33250)
When new nodes are added to the `feature_bind_extra.py` test, it succeeds if run individually, but fails if run via `test_runner` with the error message:
```
TestFramework (ERROR): Unexpected exception
Traceback (most recent call last):
File "/home/node/test/functional/test_framework/test_framework.py", line 191, in main
self.setup()
File "/home/node/test/functional/test_framework/test_framework.py", line 349, in setup
self.setup_network()
File "/home/node/build/test/functional/fe
...
💬 achow101 commented on pull request "Revert compact block cache inefficiencies":
(https://github.com/bitcoin/bitcoin/pull/33253#issuecomment-3225121798)
ACK b7b249d3adfbd3c7b0c4d0499a86300f57982972
(https://github.com/bitcoin/bitcoin/pull/33253#issuecomment-3225121798)
ACK b7b249d3adfbd3c7b0c4d0499a86300f57982972
💬 ajtowns commented on pull request "Revert compact block cache inefficiencies":
(https://github.com/bitcoin/bitcoin/pull/33253#issuecomment-3225128619)
> Edit: what is the expected memory increase because of the change?
32 bytes per mempool transaction and per extra txn (of which there are 100 by default).
A 300MB mempool might fill up with about ~100k txs (at about 440vb each on average), which would be 3.2MB total. (The 3.2MB would not be an increase overall, but would rather cause earlier eviction from the mempool, ie a ~1% reduction in capacity)
(https://github.com/bitcoin/bitcoin/pull/33253#issuecomment-3225128619)
> Edit: what is the expected memory increase because of the change?
32 bytes per mempool transaction and per extra txn (of which there are 100 by default).
A 300MB mempool might fill up with about ~100k txs (at about 440vb each on average), which would be 3.2MB total. (The 3.2MB would not be an increase overall, but would rather cause earlier eviction from the mempool, ie a ~1% reduction in capacity)