💬 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?

💬 hebasto commented on pull request "[WIP] build: remove need to test for endianness":
(https://github.com/bitcoin/bitcoin/pull/29852#issuecomment-2061969096)
> 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.

cc @sipsorcery

💬 pablomartin4btc 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-2061999792)
> Are you still working on this?

I'm about to continue, sorry for the delay!

💬 brunoerg commented on pull request "fuzz: wallet, add target for Spend":
(https://github.com/bitcoin/bitcoin/pull/28236#discussion_r1569435685)
```suggestion
// Occasionally, some random `COutPoint` are also selected.

```

👍 ryanofsky approved a pull request: "validation: improve performance of CheckBlockIndex"
(https://github.com/bitcoin/bitcoin/pull/28339#pullrequestreview-2006968441)
Code review ACK e880ee0a634f0be5079d39328fa5e118452553a9. Just rebase and suggested simplifications since the last review

💬 ryanofsky commented on pull request "validation: improve performance of CheckBlockIndex":
(https://github.com/bitcoin/bitcoin/pull/28339#discussion_r1569392358)
In commit "validation: improve performance of CheckBlockIndex" (4b05c654c1a5df294fa32a94c38001ac2b4ff2de)

It seems like this code cannot work in the case where `m_best_header` is null because `assert(pindex)` on line 5071 below will be triggered.

I think it might make sense to replace `if (m_best_header)` here with `assert(m_best_header);` to make the assumption explicit. Otherwise, I think you will need to set the `best_hdr_chain` tip to something else, like `blockman.m_block_index.begin(
...

💬 ryanofsky commented on pull request "validation: improve performance of CheckBlockIndex":
(https://github.com/bitcoin/bitcoin/pull/28339#discussion_r1569423672)
In commit "validation: improve performance of CheckBlockIndex" (4b05c654c1a5df294fa32a94c38001ac2b4ff2de)

I think this assert might be a little clearer if it replaced `m_best_header` with `best_hdr_chain.Tip()`. Both values should be identical but latter doesn't require remembering how the `best_hdr_chain` tip was assigned ~300 lines earlier in the code. Also I think the check could be stricter because if `pindex` is not null, `pindexPar` cannot be the best header. Would suggest:

```c++
/
...