💬 murchandamus commented on pull request "fuzz: wallet, add target for Spend":
(https://github.com/bitcoin/bitcoin/pull/28236#discussion_r1569167950)
If you removed the Singleton, did you perhaps not push the latest state of this PR?

💬 murchandamus commented on pull request "fuzz: wallet, add target for Spend":
(https://github.com/bitcoin/bitcoin/pull/28236#discussion_r1569173834)
Agree wtih @maflcko, since the seeds are created per target, it would be better to have a separate target for `CoinsResult`.

💬 maflcko commented on issue "Wallet fuzzing tracking issue":
(https://github.com/bitcoin/bitcoin/issues/29901#issuecomment-2061781675)
Coverage is nice, but if the fuzz test never finds any real user-facing issues, it will be useless. I don't think any wallet fuzz target found a real issue yet? Obviously coverage is the first step toward adding regression fuzz tests, so here are some ideas:

* #23444 tried to add a regression fuzz test, but it was picked up as part of https://github.com/bitcoin/bitcoin/pull/28933, and I couldn't reproduce the regression crash by re-introducing it.
* https://github.com/bitcoin/bitcoin/pull/27
...

💬 petertodd commented on pull request "tests: fix `OP_1NEGATE` handling in `CScriptOp`":
(https://github.com/bitcoin/bitcoin/pull/29589#issuecomment-2061785140)
On Wed, Apr 17, 2024 at 04:32:14AM -0700, Chris Stewart wrote:
> @dgpv @petertodd what do you think is the path forward for this?

Seems to me that one reasonable path forward is to just do nothing and not
merge these changes.

🤔 glozow reviewed a pull request: "Cluster size 2 package rbf"
(https://github.com/bitcoin/bitcoin/pull/28984#pullrequestreview-2006530259)
I think we had some docs written for doc/policy/packages.md and doc/policy/mempool_replacements.md?

💬 glozow commented on pull request "Cluster size 2 package rbf":
(https://github.com/bitcoin/bitcoin/pull/28984#discussion_r1569130553)
This seems like a merge conflict with #28970, maybe add the fix here instead?

💬 glozow commented on pull request "Cluster size 2 package rbf":
(https://github.com/bitcoin/bitcoin/pull/28984#discussion_r1569134261)
34c513554c7be97d91446126bb3ef0de2884fc19

This seems slightly inaccurate, as it wouldn't currently be in a package if `ReplacementChecks` is happening?

💬 brunoerg commented on pull request "Improve display address handling for external signer":
(https://github.com/bitcoin/bitcoin/pull/24313#issuecomment-2061802631)
> Tested with HWI 2.4.0
>
> When a Ledger Nano X is in screen saver mode, I now get the following error:
>
> ```
> '...hwi' error: Could not open client or get fingerprint information: ('0x5515', 'Error in <DefaultInsType.GET_VERSION: 1> command', '')
> ```
>
> That's what HWI returns, so can't make it much better...
>
> When the Bitcoin app is not opened on the device the error is more clear:
>
> ```
> '...hwi' error: Could not open client or get fingerprint information: Ledge
...

💬 maflcko commented on pull request "Bugfix: RPC: Check for blank rpcauth on a per-param basis":
(https://github.com/bitcoin/bitcoin/pull/29141#issuecomment-2061840567)
Are you still working on this?

💬 maflcko commented on pull request "doc: explain what the wallet password does":
(https://github.com/bitcoin/bitcoin/pull/28974#issuecomment-2061844761)
Are you still working on this?

💬 theuni commented on pull request "[WIP] build: remove need to test for endianness":
(https://github.com/bitcoin/bitcoin/pull/29852#issuecomment-2061848543)
@hebasto Is there a venue for reporting this to MSVC? They recently [patted themselves on the back for detecting similar patterns](https://devblogs.microsoft.com/cppblog/a-tour-of-4-msvc-backend-improvements/). It's a shame MSVC can't detect something that (in 2024) seems so obvious.

👍 ryanofsky approved a pull request: "kernel: De-globalize fReindex"
(https://github.com/bitcoin/bitcoin/pull/29817#pullrequestreview-2006662114)
Code review ACK d9bcbbf2293ef427b37eefca30f074be5eeeca26, but I think `reindex` /`m_reindex` naming is a little confusing so I left some suggestions below to clean it up.

I also really like stickies suggestion from https://github.com/bitcoin/bitcoin/pull/29817#pullrequestreview-2001088868 to delete the `ChainstateLoadOptions::reindex` variable and think it would be great to add it as a second commit. If you do add it, I also think it would also be good idea to add a third commit renaming `Cha
...

💬 ryanofsky commented on pull request "kernel: De-globalize fReindex":
(https://github.com/bitcoin/bitcoin/pull/29817#discussion_r1569186270)
In commit "kernel: De-globalize fReindex" (d9bcbbf2293ef427b37eefca30f074be5eeeca26)

Note: removing this line does not change behavior, because `GetBoolArg("-reindex")` is now called a few lines below on line 1501 in `ApplyArgsManOptions(args, blockman_opts)`

💬 ryanofsky commented on pull request "kernel: De-globalize fReindex":
(https://github.com/bitcoin/bitcoin/pull/29817#discussion_r1569209547)
In commit "kernel: De-globalize fReindex" (d9bcbbf2293ef427b37eefca30f074be5eeeca26)

I think `std::atomic<bool>& reindex` should be replaced by `bool reindexing` here. Calling it `reindexing` would make the name consistent with the block manager `m_reindexing` member (suggested above) which this is referencing. Avoiding std::atomic would make the function more deterministic, and avoiding the non-const reference would make it clear the value will not be modified.

💬 ryanofsky commented on pull request "kernel: De-globalize fReindex":
(https://github.com/bitcoin/bitcoin/pull/29817#discussion_r1569200553)
In commit "kernel: De-globalize fReindex" (d9bcbbf2293ef427b37eefca30f074be5eeeca26)

I don't think `m_reindex` is good name for this variable because it is easy to get confused with the `reindex` option which reflects whether reindexing was requested, not whether it is currently happening. Would suggest calling this `m_reindexing` and adding a comment that this reflects whether reindexing is currently happening, and is set to true when reindexing is requested, and false when reindexing comple
...

💬 instagibbs commented on pull request "Cluster size 2 package rbf":
(https://github.com/bitcoin/bitcoin/pull/28984#discussion_r1569241334)
"in a package"?

💬 brunoerg commented on pull request "Improve display address handling for external signer":
(https://github.com/bitcoin/bitcoin/pull/24313#issuecomment-2061908690)
I did same test as https://github.com/bitcoin/bitcoin/pull/24313#issuecomment-1867691764 but with the latest version of HWI.

```sh
./src/bitcoin-cli -rpcwallet=ledger7 walletdisplayaddress "bc1qfshzjl770c6qz560cade6ynswj2a3scqzm4a4m"
error code: -1
error message:
'/Users/brunogarcia/projects/HWI/hwi.py' error: Could not open client or get fingerprint information: Ledger is not in either the Bitcoin or Bitcoin Testnet app
```

```sh
./src/bitcoin-cli -rpcwallet=ledger7 walletdisplay
...

💬 glozow commented on pull request "Cluster size 2 package rbf":
(https://github.com/bitcoin/bitcoin/pull/28984#discussion_r1569270142)
Maybe you can also check the topology/cluster size of this tx in mempool?

💬 maflcko commented on pull request "cli: improve error message on multiwallet and add validation to cli-side commands":
(https://github.com/bitcoin/bitcoin/pull/26990#issuecomment-2061931410)
Are you still working on this?

💬 maflcko commented on pull request "fuzz: wallet, add target for Spend":
(https://github.com/bitcoin/bitcoin/pull/28236#issuecomment-2061935258)
Are you still working on this?