💬 instagibbs commented on pull request "Cluster mempool implementation":
(https://github.com/bitcoin/bitcoin/pull/28676#discussion_r2369478308)
as of "Rework RBF and TRUC validation" this check is now redundant I believe
(https://github.com/bitcoin/bitcoin/pull/28676#discussion_r2369478308)
as of "Rework RBF and TRUC validation" this check is now redundant I believe
💬 instagibbs commented on pull request "Cluster mempool implementation":
(https://github.com/bitcoin/bitcoin/pull/28676#discussion_r2369521026)
00c02b42c6199a6c46573304761056082490c7d4
While we're here: Suggestion to make it more clear as to the roles of the various TRUC checks and that everything works once dependencies are attached.
```Suggestion
// Either sibling eviction was needed, or package based check should always pass for single tx given SingleTRUCChecks already passed
Assume(ws.m_sibling_eviction || !PackageTRUCChecks(m_pool, ws.m_ptx, ws.m_vsize, {ws.m_ptx}, ws.m_parents));
```
(https://github.com/bitcoin/bitcoin/pull/28676#discussion_r2369521026)
00c02b42c6199a6c46573304761056082490c7d4
While we're here: Suggestion to make it more clear as to the roles of the various TRUC checks and that everything works once dependencies are attached.
```Suggestion
// Either sibling eviction was needed, or package based check should always pass for single tx given SingleTRUCChecks already passed
Assume(ws.m_sibling_eviction || !PackageTRUCChecks(m_pool, ws.m_ptx, ws.m_vsize, {ws.m_ptx}, ws.m_parents));
```
💬 instagibbs commented on pull request "Cluster mempool implementation":
(https://github.com/bitcoin/bitcoin/pull/28676#discussion_r2369593431)
I agree after some thought that txgraph-based checking is too difficult to do, at least for now. I can foresee some weird complications and I'm unsure I could even describe which series of steps would be correct vs our more bespoke approach currently which sidesteps it.
Re-arrangement of checks seems correct, will take a fresh look at the approach soon.
related: Documentation for `PreChecks` needs to be updated as it does not check package(cluster) limits any longer aside from TRUC checks.
...
(https://github.com/bitcoin/bitcoin/pull/28676#discussion_r2369593431)
I agree after some thought that txgraph-based checking is too difficult to do, at least for now. I can foresee some weird complications and I'm unsure I could even describe which series of steps would be correct vs our more bespoke approach currently which sidesteps it.
Re-arrangement of checks seems correct, will take a fresh look at the approach soon.
related: Documentation for `PreChecks` needs to be updated as it does not check package(cluster) limits any longer aside from TRUC checks.
...
💬 Crypt-iQ commented on pull request "docs: Undeprecate datacarrier and datacarriersize configuration options":
(https://github.com/bitcoin/bitcoin/pull/33453#issuecomment-3320444838)
@ajtowns wrote:
> For comparison, the difficulty increase from early May to now (given the same hashrate) implies a decrease in revenue of 19.5%, more than 100x as much. I think it makes sense to set defaults to maximise even small amounts of revenue, but that's a "picking up pennies" saving, not a "small miners will be put out of business" situation.
Is the argument that poor compact block reconstruction rates leads to miner centralization overblown then if the loss in revenue will be dwa
...
(https://github.com/bitcoin/bitcoin/pull/33453#issuecomment-3320444838)
@ajtowns wrote:
> For comparison, the difficulty increase from early May to now (given the same hashrate) implies a decrease in revenue of 19.5%, more than 100x as much. I think it makes sense to set defaults to maximise even small amounts of revenue, but that's a "picking up pennies" saving, not a "small miners will be put out of business" situation.
Is the argument that poor compact block reconstruction rates leads to miner centralization overblown then if the loss in revenue will be dwa
...
💬 mzumsande commented on pull request "wallet: reduce unconditional logging during load":
(https://github.com/bitcoin/bitcoin/pull/33299#issuecomment-3320482300)
> This version doesn't work properly and produces the same issue it's trying to fix as every call to MakeWalletDatabase() within conditionals in VerifyWallets() (src/wallet/load.cpp), ends up also calling the SQLiteDatabase::Cleanup() in the destructor which does --g_sqlite_count.
Logging aside, I wonder if it's useful and intended to do all the existing setup including `sqlite3_initialize()`, followed by `sqlite3_shutdown()` multiple times during init for each wallet checked during `VerifyWa
...
(https://github.com/bitcoin/bitcoin/pull/33299#issuecomment-3320482300)
> This version doesn't work properly and produces the same issue it's trying to fix as every call to MakeWalletDatabase() within conditionals in VerifyWallets() (src/wallet/load.cpp), ends up also calling the SQLiteDatabase::Cleanup() in the destructor which does --g_sqlite_count.
Logging aside, I wonder if it's useful and intended to do all the existing setup including `sqlite3_initialize()`, followed by `sqlite3_shutdown()` multiple times during init for each wallet checked during `VerifyWa
...
👍 ryanofsky approved a pull request: "cli: Handle arguments that can be either JSON or string"
(https://github.com/bitcoin/bitcoin/pull/33230#pullrequestreview-3254240166)
Code review ACK aabf1f6093892d372544c8b5d55d260699dab69c. This should be a nice usability improvement. I left a suggestion for making the test stronger and confirming client and server types are consistent, and I think it would not be hard to implement, but it's not critical.
I do think this approach is a pretty reasonable way to avoid the need for calling `bitcoin-cli` with extra quotes around `ParseHashOrHeight` parameters here, and around addresses in #32468. And it's probably the best app
...
(https://github.com/bitcoin/bitcoin/pull/33230#pullrequestreview-3254240166)
Code review ACK aabf1f6093892d372544c8b5d55d260699dab69c. This should be a nice usability improvement. I left a suggestion for making the test stronger and confirming client and server types are consistent, and I think it would not be hard to implement, but it's not critical.
I do think this approach is a pretty reasonable way to avoid the need for calling `bitcoin-cli` with extra quotes around `ParseHashOrHeight` parameters here, and around addresses in #32468. And it's probably the best app
...
💬 ryanofsky commented on pull request "cli: Handle arguments that can be either JSON or string":
(https://github.com/bitcoin/bitcoin/pull/33230#discussion_r2369570345)
> This is here because the test fails without it.
It seems like the test could be improved, but it would expand the PR a little. I think ideally type of `hash_or_height` `rollback` parameters server side would not be `NUM`, but a more accurate type like `NUM_OR_STR`. (#32468 could use `ARR_OR_STR`.)
The server could return this information back to the test either by modifying the current type column:
https://github.com/bitcoin/bitcoin/blob/34fefb633584ecd803b01209756f2bef412f1cb1/src/rp
...
(https://github.com/bitcoin/bitcoin/pull/33230#discussion_r2369570345)
> This is here because the test fails without it.
It seems like the test could be improved, but it would expand the PR a little. I think ideally type of `hash_or_height` `rollback` parameters server side would not be `NUM`, but a more accurate type like `NUM_OR_STR`. (#32468 could use `ARR_OR_STR`.)
The server could return this information back to the test either by modifying the current type column:
https://github.com/bitcoin/bitcoin/blob/34fefb633584ecd803b01209756f2bef412f1cb1/src/rp
...
💬 furszy commented on pull request "wallet: reduce unconditional logging during load":
(https://github.com/bitcoin/bitcoin/pull/33299#issuecomment-3320690729)
> > This version doesn't work properly and produces the same issue it's trying to fix as every call to MakeWalletDatabase() within conditionals in VerifyWallets() (src/wallet/load.cpp), ends up also calling the SQLiteDatabase::Cleanup() in the destructor which does --g_sqlite_count.
>
> Logging aside, I wonder if it's useful and intended to do all the existing setup including `sqlite3_initialize()`, followed by `sqlite3_shutdown()` multiple times during init for each wallet checked during `Ve
...
(https://github.com/bitcoin/bitcoin/pull/33299#issuecomment-3320690729)
> > This version doesn't work properly and produces the same issue it's trying to fix as every call to MakeWalletDatabase() within conditionals in VerifyWallets() (src/wallet/load.cpp), ends up also calling the SQLiteDatabase::Cleanup() in the destructor which does --g_sqlite_count.
>
> Logging aside, I wonder if it's useful and intended to do all the existing setup including `sqlite3_initialize()`, followed by `sqlite3_shutdown()` multiple times during init for each wallet checked during `Ve
...
💬 hodlinator commented on pull request "msvc: Update vcpkg manifest":
(https://github.com/bitcoin/bitcoin/pull/33408#issuecomment-3320698708)
ACK ef20c2d11d960bf915f88cdb2ceac2184e4aec10
Verified vcpkg tags / commit hashes.
nit: Discovered image blurring/smoothing worsened from master to PR. Here are two native Windows builds:
#### 947bed28fe62cd6c27baace5ae7c2b6955ee6379 (parent commit of PR on master, built natively on Windows)
<img width="433" height="349" alt="947bed28fe62cd6c27baace5ae7c2b6955ee6379" src="https://github.com/user-attachments/assets/8a78e493-4a36-424c-8716-3a3679f09c4c" />
#### ef20c2d11d960bf915f88cdb
...
(https://github.com/bitcoin/bitcoin/pull/33408#issuecomment-3320698708)
ACK ef20c2d11d960bf915f88cdb2ceac2184e4aec10
Verified vcpkg tags / commit hashes.
nit: Discovered image blurring/smoothing worsened from master to PR. Here are two native Windows builds:
#### 947bed28fe62cd6c27baace5ae7c2b6955ee6379 (parent commit of PR on master, built natively on Windows)
<img width="433" height="349" alt="947bed28fe62cd6c27baace5ae7c2b6955ee6379" src="https://github.com/user-attachments/assets/8a78e493-4a36-424c-8716-3a3679f09c4c" />
#### ef20c2d11d960bf915f88cdb
...
💬 pablomartin4btc commented on pull request "wallet: reduce unconditional logging during load":
(https://github.com/bitcoin/bitcoin/pull/33299#issuecomment-3320719513)
> Logging aside, I wonder if it's useful and intended to do all the existing setup including `sqlite3_initialize()`, followed by `sqlite3_shutdown()` multiple times during init for each wallet checked during `VerifyWallets()`? Seems like the intention of `g_sqlite_count` is to do it only once?!
So, we have one sqlite (/ walletdb/ count) obj instance per wallet (/ wallet file), so during init we check first if there's a default wallet and then also check the wallets in the settings config (wha
...
(https://github.com/bitcoin/bitcoin/pull/33299#issuecomment-3320719513)
> Logging aside, I wonder if it's useful and intended to do all the existing setup including `sqlite3_initialize()`, followed by `sqlite3_shutdown()` multiple times during init for each wallet checked during `VerifyWallets()`? Seems like the intention of `g_sqlite_count` is to do it only once?!
So, we have one sqlite (/ walletdb/ count) obj instance per wallet (/ wallet file), so during init we check first if there's a default wallet and then also check the wallets in the settings config (wha
...
🤔 hebasto reviewed a pull request: "build: Remove deprecated CMAKE_SKIP_BUILD_RPATH and SKIP_BUILD_RPATH settings"
(https://github.com/bitcoin/bitcoin/pull/33247#pullrequestreview-3254612614)
Approach ACK 07027afa155f8ec9715c761d80630b31723c2c32.
The comment "`TODO: The `CMAKE_SKIP_BUILD_RPATH` variable setting can be deleted...`" does not imply that anything is deprecated. Could you please update the PR description and commit message accordingly?
(https://github.com/bitcoin/bitcoin/pull/33247#pullrequestreview-3254612614)
Approach ACK 07027afa155f8ec9715c761d80630b31723c2c32.
The comment "`TODO: The `CMAKE_SKIP_BUILD_RPATH` variable setting can be deleted...`" does not imply that anything is deprecated. Could you please update the PR description and commit message accordingly?
💬 hebasto commented on pull request "build: Remove deprecated CMAKE_SKIP_BUILD_RPATH and SKIP_BUILD_RPATH settings":
(https://github.com/bitcoin/bitcoin/pull/33247#discussion_r2369869582)
It's outside the scope of this PR, but `CMAKE_SKIP_INSTALL_RPATH` should be set in the Guix script, keeping the build system free of hardcoded behaviour.
(https://github.com/bitcoin/bitcoin/pull/33247#discussion_r2369869582)
It's outside the scope of this PR, but `CMAKE_SKIP_INSTALL_RPATH` should be set in the Guix script, keeping the build system free of hardcoded behaviour.
💬 hodlinator commented on pull request "qa: Improvements to debug_assert_log + busy_wait_for_debug_log":
(https://github.com/bitcoin/bitcoin/pull/33423#issuecomment-3320876003)
Latest push has somewhat simpler code (no `found`-list). Tested performance again, similarly to https://github.com/bitcoin/bitcoin/pull/33423#pullrequestreview-3244741286:
```
₿ git co 2025/09/assert_debug_log_rebased~3
₿ hyperfine -r 30 -N ./build/test/functional/feature_assumevalid.py
Benchmark 1: ./build/test/functional/feature_assumevalid.py
Time (mean ± σ): 6.034 s ± 0.047 s [User: 4.260 s, System: 0.488 s]
Range (min … max): 5.964 s … 6.146 s 30 runs
₿ git co
...
(https://github.com/bitcoin/bitcoin/pull/33423#issuecomment-3320876003)
Latest push has somewhat simpler code (no `found`-list). Tested performance again, similarly to https://github.com/bitcoin/bitcoin/pull/33423#pullrequestreview-3244741286:
```
₿ git co 2025/09/assert_debug_log_rebased~3
₿ hyperfine -r 30 -N ./build/test/functional/feature_assumevalid.py
Benchmark 1: ./build/test/functional/feature_assumevalid.py
Time (mean ± σ): 6.034 s ± 0.047 s [User: 4.260 s, System: 0.488 s]
Range (min … max): 5.964 s … 6.146 s 30 runs
₿ git co
...
📝 theStack opened a pull request: "doc: remove unrelated `bitcoin-wallet` binary from `libbitcoin_ipc` description"
(https://github.com/bitcoin/bitcoin/pull/33459)
`bitcoin-wallet` as-is is merely an offline wallet inspection tool (introduced more than 9 years ago in PR #13926) that doesn't have any relation with IPC/multiprocess, so remove it from the list of binaries that use `libbitcoin_ipc`.
(https://github.com/bitcoin/bitcoin/pull/33459)
`bitcoin-wallet` as-is is merely an offline wallet inspection tool (introduced more than 9 years ago in PR #13926) that doesn't have any relation with IPC/multiprocess, so remove it from the list of binaries that use `libbitcoin_ipc`.
👍 ryanofsky approved a pull request: "rpc: Handle -named argument parsing where '=' character is used"
(https://github.com/bitcoin/bitcoin/pull/32821#pullrequestreview-3254620368)
Code review ACK ad6d740623f01bbe90cbf328590ccfae602c2a3f, just updating documentation and converting class into a namespace since last review.
I left some comments below, but feel free to ignore them, they are just explaining previous suggestions not adding anything new.
(https://github.com/bitcoin/bitcoin/pull/32821#pullrequestreview-3254620368)
Code review ACK ad6d740623f01bbe90cbf328590ccfae602c2a3f, just updating documentation and converting class into a namespace since last review.
I left some comments below, but feel free to ignore them, they are just explaining previous suggestions not adding anything new.
💬 ryanofsky commented on pull request "rpc: Handle -named argument parsing where '=' character is used":
(https://github.com/bitcoin/bitcoin/pull/32821#discussion_r2369951983)
In commit "rpc: Handle -named argument parsing where '=' character is used" (8eafb1e9ee4fff3a9ab4a7dc899bd2d57c29d04f)
New example below is nice, and feel free to ignore this feedback, but IMO lines 437-447 are not as good as the earlier suggestion for a few reasons:
- They is saying what code is doing without reasons for doing those things.
- They are not acknowledging the -named syntax is ambiguous, which seems an important and non-obvious fact.
- The first two sentences seem to contra
...
(https://github.com/bitcoin/bitcoin/pull/32821#discussion_r2369951983)
In commit "rpc: Handle -named argument parsing where '=' character is used" (8eafb1e9ee4fff3a9ab4a7dc899bd2d57c29d04f)
New example below is nice, and feel free to ignore this feedback, but IMO lines 437-447 are not as good as the earlier suggestion for a few reasons:
- They is saying what code is doing without reasons for doing those things.
- They are not acknowledging the -named syntax is ambiguous, which seems an important and non-obvious fact.
- The first two sentences seem to contra
...
💬 ryanofsky commented on pull request "rpc: Handle -named argument parsing where '=' character is used":
(https://github.com/bitcoin/bitcoin/pull/32821#discussion_r2369876273)
In commit "rpc: Handle -named argument parsing where '=' character is used" (8eafb1e9ee4fff3a9ab4a7dc899bd2d57c29d04f)
Note: namespace contents is typically not indented: https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#coding-style-c
I also think entire namespace could be replaced with a `ParamFind` function
https://github.com/bitcoin/bitcoin/blob/da919fb63c519ac91c9704c58f1bbe522d7a8879/src/rpc/client.cpp#L384-L389
Which would deduplicate and remove a level of
...
(https://github.com/bitcoin/bitcoin/pull/32821#discussion_r2369876273)
In commit "rpc: Handle -named argument parsing where '=' character is used" (8eafb1e9ee4fff3a9ab4a7dc899bd2d57c29d04f)
Note: namespace contents is typically not indented: https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#coding-style-c
I also think entire namespace could be replaced with a `ParamFind` function
https://github.com/bitcoin/bitcoin/blob/da919fb63c519ac91c9704c58f1bbe522d7a8879/src/rpc/client.cpp#L384-L389
Which would deduplicate and remove a level of
...
💬 ryanofsky commented on pull request "rpc: Handle -named argument parsing where '=' character is used":
(https://github.com/bitcoin/bitcoin/pull/32821#discussion_r2369939396)
In commit "rpc: Handle -named argument parsing where '=' character is used" (8eafb1e9ee4fff3a9ab4a7dc899bd2d57c29d04f)
Woud suggest replacing `parameter with PARAM_NAME="my".` with `parameter named "my"` so this is shorter and more understandable without the previous context.
(https://github.com/bitcoin/bitcoin/pull/32821#discussion_r2369939396)
In commit "rpc: Handle -named argument parsing where '=' character is used" (8eafb1e9ee4fff3a9ab4a7dc899bd2d57c29d04f)
Woud suggest replacing `parameter with PARAM_NAME="my".` with `parameter named "my"` so this is shorter and more understandable without the previous context.
💬 moonsettler commented on pull request "docs: Undeprecate datacarrier and datacarriersize configuration options":
(https://github.com/bitcoin/bitcoin/pull/33453#issuecomment-3321131775)
> I would prefer a PR improving the deprecation message to make it clear that we are discouraging the use of the option, but not planning to remove it (for the moment).
That is also fine. I have not changed my mind on this, I considered removing the option a bad move to begin with. This option being configurable made even less practical sense in the past than it has going forward.
(https://github.com/bitcoin/bitcoin/pull/33453#issuecomment-3321131775)
> I would prefer a PR improving the deprecation message to make it clear that we are discouraging the use of the option, but not planning to remove it (for the moment).
That is also fine. I have not changed my mind on this, I considered removing the option a bad move to begin with. This option being configurable made even less practical sense in the past than it has going forward.
👍 hodlinator approved a pull request: "Modernize use of UTF-8 in Windows code"
(https://github.com/bitcoin/bitcoin/pull/32380#pullrequestreview-3254889896)
ACK b74a92cdb2c4d9871865a67162f47f97fb7e642c
Latest push adds missing/previously avoided call to `SetupEnvironment()` (discovered in https://github.com/bitcoin/bitcoin/pull/32380#discussion_r2367130713), this entails also adding a dependency from bitcoin.exe on bitcoin_common.
Tested Windows-native build, running `bitcoin node`.
(https://github.com/bitcoin/bitcoin/pull/32380#pullrequestreview-3254889896)
ACK b74a92cdb2c4d9871865a67162f47f97fb7e642c
Latest push adds missing/previously avoided call to `SetupEnvironment()` (discovered in https://github.com/bitcoin/bitcoin/pull/32380#discussion_r2367130713), this entails also adding a dependency from bitcoin.exe on bitcoin_common.
Tested Windows-native build, running `bitcoin node`.