💬 rkrux commented on pull request "wallet, rpc, doc: various legacy wallet removal cleanups in RPCs":
(https://github.com/bitcoin/bitcoin/pull/32596#discussion_r2104780565)
Nice, thanks: https://github.com/bitcoin/bitcoin/pull/31250#discussion_r2042020967
(https://github.com/bitcoin/bitcoin/pull/32596#discussion_r2104780565)
Nice, thanks: https://github.com/bitcoin/bitcoin/pull/31250#discussion_r2042020967
💬 rkrux commented on pull request "wallet, rpc, doc: various legacy wallet removal cleanups in RPCs":
(https://github.com/bitcoin/bitcoin/pull/32596#discussion_r2104775279)
I assume this is a breaking change but fine because it will go in the same release that contains the removal of legacy wallets as well.
(https://github.com/bitcoin/bitcoin/pull/32596#discussion_r2104775279)
I assume this is a breaking change but fine because it will go in the same release that contains the removal of legacy wallets as well.
💬 maflcko commented on pull request "test: Fix `system_tests/run_command` test":
(https://github.com/bitcoin/bitcoin/pull/32601#discussion_r2104782256)
Does quoting not work?
(https://github.com/bitcoin/bitcoin/pull/32601#discussion_r2104782256)
Does quoting not work?
💬 hebasto commented on pull request "test: Fix `system_tests/run_command` test":
(https://github.com/bitcoin/bitcoin/pull/32601#discussion_r2104784521)
> Does quoting not work?
Not for me.
(https://github.com/bitcoin/bitcoin/pull/32601#discussion_r2104784521)
> Does quoting not work?
Not for me.
📝 marcofleon opened a pull request: "fuzz: Add target for coins database"
(https://github.com/bitcoin/bitcoin/pull/32602)
This reopens https://github.com/bitcoin/bitcoin/pull/28216.
The current `coins_view` target only tests `CCoinsViewCache` using a basic `CCoinsView` instance. The addition of the `coins_view_db` target enables testing with an actual `CCoinsViewDB` as the backend.
(https://github.com/bitcoin/bitcoin/pull/32602)
This reopens https://github.com/bitcoin/bitcoin/pull/28216.
The current `coins_view` target only tests `CCoinsViewCache` using a basic `CCoinsView` instance. The addition of the `coins_view_db` target enables testing with an actual `CCoinsViewDB` as the backend.
💬 hebasto commented on pull request "test: Fix `system_tests/run_command` test":
(https://github.com/bitcoin/bitcoin/pull/32601#discussion_r2104788947)
`subprocess` uses space and tab as delimiters:https://github.com/bitcoin/bitcoin/blob/2df824f4e62b6bc569044819cd64f66f3839ba13/src/util/subprocess.h#L307-L334
(https://github.com/bitcoin/bitcoin/pull/32601#discussion_r2104788947)
`subprocess` uses space and tab as delimiters:https://github.com/bitcoin/bitcoin/blob/2df824f4e62b6bc569044819cd64f66f3839ba13/src/util/subprocess.h#L307-L334
💬 marcofleon commented on pull request "fuzz: Add target for coins database":
(https://github.com/bitcoin/bitcoin/pull/32602#issuecomment-2904747826)
The coverage between the two targets can be compared [here](https://marcofleon.github.io/coverage/coins_view/) and [here](https://marcofleon.github.io/coverage/coins_view_db/).
Instability was a bit of a concern in the last PR, but was answered in https://github.com/bitcoin/bitcoin/pull/28216#issuecomment-2754900599.
(https://github.com/bitcoin/bitcoin/pull/32602#issuecomment-2904747826)
The coverage between the two targets can be compared [here](https://marcofleon.github.io/coverage/coins_view/) and [here](https://marcofleon.github.io/coverage/coins_view_db/).
Instability was a bit of a concern in the last PR, but was answered in https://github.com/bitcoin/bitcoin/pull/28216#issuecomment-2754900599.
💬 fanquake commented on pull request "fuzz: a new target for the coins database":
(https://github.com/bitcoin/bitcoin/pull/28216#issuecomment-2904748427)
Picked up in #32602.
(https://github.com/bitcoin/bitcoin/pull/28216#issuecomment-2904748427)
Picked up in #32602.
💬 davidgumberg commented on pull request "ci: remove 3rd party js from windows dll gha job":
(https://github.com/bitcoin/bitcoin/pull/32513#issuecomment-2904755024)
This approach here should also be applied to replace the `Find mt.exe tool` step in the `Windows, test cross-built` job.
(https://github.com/bitcoin/bitcoin/pull/32513#issuecomment-2904755024)
This approach here should also be applied to replace the `Find mt.exe tool` step in the `Windows, test cross-built` job.
💬 theStack commented on pull request "wallet, rpc, doc: various legacy wallet removal cleanups in RPCs":
(https://github.com/bitcoin/bitcoin/pull/32596#discussion_r2104795226)
Ah interesting, wasn't aware that this was proposed already in an earlier PR. Added a link to your comment in the PR description.
(https://github.com/bitcoin/bitcoin/pull/32596#discussion_r2104795226)
Ah interesting, wasn't aware that this was proposed already in an earlier PR. Added a link to your comment in the PR description.
💬 gmaxwell commented on issue "compact block fingerprinting":
(https://github.com/bitcoin/bitcoin/issues/28272#issuecomment-2904766683)
FWIW, this is solvable by changing to more fibre like methods for block transmission. Because FEC coded missing data doesn't identify what was missing.
So I had *thought* the code would check if there were too many missing transactions and just request a full block if all were missing. That doesn't address the general attack (since the attacker could admit some real txn that are in the block) but it would mostly address the blocksonly case (and could easily get a if blocksonly check so that i
...
(https://github.com/bitcoin/bitcoin/issues/28272#issuecomment-2904766683)
FWIW, this is solvable by changing to more fibre like methods for block transmission. Because FEC coded missing data doesn't identify what was missing.
So I had *thought* the code would check if there were too many missing transactions and just request a full block if all were missing. That doesn't address the general attack (since the attacker could admit some real txn that are in the block) but it would mostly address the blocksonly case (and could easily get a if blocksonly check so that i
...
💬 theStack commented on pull request "wallet, rpc, doc: various legacy wallet removal cleanups in RPCs":
(https://github.com/bitcoin/bitcoin/pull/32596#discussion_r2104807129)
As this is an optional return field, I wouldn't consider its removal as a breaking change, considering that the condition for returning a value (i.e. executing the RPC on a legacy wallet) can't be fulfilled anymore. Curious about other opinions though; I guess one could argue that there is still a point in mentioning the removal in the release notes?
(https://github.com/bitcoin/bitcoin/pull/32596#discussion_r2104807129)
As this is an optional return field, I wouldn't consider its removal as a breaking change, considering that the condition for returning a value (i.e. executing the RPC on a legacy wallet) can't be fulfilled anymore. Curious about other opinions though; I guess one could argue that there is still a point in mentioning the removal in the release notes?
📝 rkrux opened a pull request: "wallet, rpc: clarify wallet version in getwalletinfo help"
(https://github.com/bitcoin/bitcoin/pull/32603)
I noted in review of a previous PR here: https://github.com/bitcoin/bitcoin/pull/32553#pullrequestreview-2853743852.
Relaying the same information in the RPC help that's present in a code comment: https://github.com/bitcoin/bitcoin/blob/af65fd1a333011137dafd3df9a51704fd319feb4/src/wallet/wallet.h#L809-L810
<!--
*** Please remove the following help text before submitting: ***
Pull requests without a rationale and clear improvement may be closed
immediately.
GUI-related pull requests
...
(https://github.com/bitcoin/bitcoin/pull/32603)
I noted in review of a previous PR here: https://github.com/bitcoin/bitcoin/pull/32553#pullrequestreview-2853743852.
Relaying the same information in the RPC help that's present in a code comment: https://github.com/bitcoin/bitcoin/blob/af65fd1a333011137dafd3df9a51704fd319feb4/src/wallet/wallet.h#L809-L810
<!--
*** Please remove the following help text before submitting: ***
Pull requests without a rationale and clear improvement may be closed
immediately.
GUI-related pull requests
...
🤔 l0rinc reviewed a pull request: "fuzz: Add target for coins database"
(https://github.com/bitcoin/bitcoin/pull/32602#pullrequestreview-2864770398)
Concept ACK
Quickly reviewed it again, will do it more deeply later - do we still need the `cachedCoinsUsage` comment after https://github.com/bitcoin/bitcoin/pull/32313, which should fix the accounting bug instead of working around it?
(https://github.com/bitcoin/bitcoin/pull/32602#pullrequestreview-2864770398)
Concept ACK
Quickly reviewed it again, will do it more deeply later - do we still need the `cachedCoinsUsage` comment after https://github.com/bitcoin/bitcoin/pull/32313, which should fix the accounting bug instead of working around it?
💬 l0rinc commented on pull request "fuzz: Add target for coins database":
(https://github.com/bitcoin/bitcoin/pull/32602#discussion_r2104799482)
There's a helper introduced since:
```suggestion
.cache_bytes = 1_MiB,
```
(https://github.com/bitcoin/bitcoin/pull/32602#discussion_r2104799482)
There's a helper introduced since:
```suggestion
.cache_bytes = 1_MiB,
```
💬 l0rinc commented on pull request "fuzz: Add target for coins database":
(https://github.com/bitcoin/bitcoin/pull/32602#discussion_r2104802538)
I still think this comment is way to verbose, we don't need that much context here.
Is this still needed after [`327ee45` (#32313)](https://github.com/bitcoin/bitcoin/pull/32313/commits/327ee453cab9d7c8482f74b195f2be6c6f1a4cc9) where we might not get into the described situation anymore since `cachedCoinsUsage` update only happens after the mentioned exception now.
(https://github.com/bitcoin/bitcoin/pull/32602#discussion_r2104802538)
I still think this comment is way to verbose, we don't need that much context here.
Is this still needed after [`327ee45` (#32313)](https://github.com/bitcoin/bitcoin/pull/32313/commits/327ee453cab9d7c8482f74b195f2be6c6f1a4cc9) where we might not get into the described situation anymore since `cachedCoinsUsage` update only happens after the mentioned exception now.
💬 l0rinc commented on pull request "fuzz: Add target for coins database":
(https://github.com/bitcoin/bitcoin/pull/32602#discussion_r2104798281)
nit: in other cases in this file the param hints are written as:
```suggestion
TestCoinsView(fuzzed_data_provider, coins_db, /*is_db=*/ true);
```
(https://github.com/bitcoin/bitcoin/pull/32602#discussion_r2104798281)
nit: in other cases in this file the param hints are written as:
```suggestion
TestCoinsView(fuzzed_data_provider, coins_db, /*is_db=*/ true);
```
💬 l0rinc commented on pull request "fuzz: Add target for coins database":
(https://github.com/bitcoin/bitcoin/pull/32602#discussion_r2104812882)
The default value for `is_db` may not be intuitive, can we make it explicit instead?
(https://github.com/bitcoin/bitcoin/pull/32602#discussion_r2104812882)
The default value for `is_db` may not be intuitive, can we make it explicit instead?
💬 l0rinc commented on pull request "fuzz: Add target for coins database":
(https://github.com/bitcoin/bitcoin/pull/32602#discussion_r2104819427)
can you please add a short comment to the `best_block = uint256::ONE` cases?
(https://github.com/bitcoin/bitcoin/pull/32602#discussion_r2104819427)
can you please add a short comment to the `best_block = uint256::ONE` cases?
💬 rkrux commented on pull request "wallet, rpc, doc: various legacy wallet removal cleanups in RPCs":
(https://github.com/bitcoin/bitcoin/pull/32596#discussion_r2104831591)
Ah, I note now that its presence was conditional on legacy wallets.
(https://github.com/bitcoin/bitcoin/pull/32596#discussion_r2104831591)
Ah, I note now that its presence was conditional on legacy wallets.