💬 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)`
(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.
(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
...
(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"?
(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
...
(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?
(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?
(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?
(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
(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!
(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.
```
(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
(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(
...
(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++
/
...
(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++
/
...
💬 sipsorcery commented on pull request "[WIP] build: remove need to test for endianness":
(https://github.com/bitcoin/bitcoin/pull/29852#issuecomment-2062117184)
> > 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
Most likely fruitless but can't hurt to ask.
https://x.com/sipsorcery/status/1780687316522266853
> > 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
Most likely fruitless but can't hurt to ask.
https://x.com/sipsorcery/status/1780687316522266853
![image](https://github.com/bitcoin/bitcoin/assets/197660/e15514f4-4669-42e3-96a2-7b21429f5a
...
💬 furszy commented on pull request "index: race fix, lock cs_main while 'm_synced' is subject to change":
(https://github.com/bitcoin/bitcoin/pull/29867#issuecomment-2062141621)
Little delayed but here again. I'm also okay with both options. I'm feeling a bit more inclined towards option 3, primarily for the added comments.
Also, just to share some extra thoughts around this issue, another -bad- option to hold a minimal `cs_main` lock could be to keep the current flow in master and connect missing intermediate blocks inside `BlockConnected` only the first time it is called after setting `m_synced`. This does not require chain access nor any extra cs_main lock, as we
...
(https://github.com/bitcoin/bitcoin/pull/29867#issuecomment-2062141621)
Little delayed but here again. I'm also okay with both options. I'm feeling a bit more inclined towards option 3, primarily for the added comments.
Also, just to share some extra thoughts around this issue, another -bad- option to hold a minimal `cs_main` lock could be to keep the current flow in master and connect missing intermediate blocks inside `BlockConnected` only the first time it is called after setting `m_synced`. This does not require chain access nor any extra cs_main lock, as we
...
🤔 sr-gi reviewed a pull request: "p2p: opportunistically accept 1-parent-1-child packages"
(https://github.com/bitcoin/bitcoin/pull/28970#pullrequestreview-2004484626)
First pass. Reviewed up to 91f4efa420958a93f4620379f8830231f276b23b
(https://github.com/bitcoin/bitcoin/pull/28970#pullrequestreview-2004484626)
First pass. Reviewed up to 91f4efa420958a93f4620379f8830231f276b23b
💬 sr-gi commented on pull request "p2p: opportunistically accept 1-parent-1-child packages":
(https://github.com/bitcoin/bitcoin/pull/28970#discussion_r1569085792)
In: 54d0c78d104fb5412a194816590a06cad8cadf80
Calling `ToUint256()` shouldn't be needed
(https://github.com/bitcoin/bitcoin/pull/28970#discussion_r1569085792)
In: 54d0c78d104fb5412a194816590a06cad8cadf80
Calling `ToUint256()` shouldn't be needed
💬 sr-gi commented on pull request "p2p: opportunistically accept 1-parent-1-child packages":
(https://github.com/bitcoin/bitcoin/pull/28970#discussion_r1569100856)
In: 54d0c78d104fb5412a194816590a06cad8cadf80
I'm struggling to see the usefulness of this. You are showing that, for the provided transactions, the ordering may be different based on the representation used (`wtxid`/`txid`/`ToUint256`/`GetHex`), but I don't think this clearly shows that the package hash is using one or the other.
You are already proving that the order is the one you are expecting by manually computing `calculated_hash_456`. You could also create different orderings based o
...
(https://github.com/bitcoin/bitcoin/pull/28970#discussion_r1569100856)
In: 54d0c78d104fb5412a194816590a06cad8cadf80
I'm struggling to see the usefulness of this. You are showing that, for the provided transactions, the ordering may be different based on the representation used (`wtxid`/`txid`/`ToUint256`/`GetHex`), but I don't think this clearly shows that the package hash is using one or the other.
You are already proving that the order is the one you are expecting by manually computing `calculated_hash_456`. You could also create different orderings based o
...
💬 sr-gi commented on pull request "p2p: opportunistically accept 1-parent-1-child packages":
(https://github.com/bitcoin/bitcoin/pull/28970#discussion_r1568990742)
In: 54d0c78d104fb5412a194816590a06cad8cadf80
I don't see what the point of building the `txid` from a literal in this way and comparing it to the one obtained via `GetHash()` is. I'm guessing you're trying to make the point that the txids are actually what you are claiming them to be (as opposed to just writing them in a comment), so the reader can manually check the difference between internal and human-readable lexicographic ordering. Is that really necessary?
(https://github.com/bitcoin/bitcoin/pull/28970#discussion_r1568990742)
In: 54d0c78d104fb5412a194816590a06cad8cadf80
I don't see what the point of building the `txid` from a literal in this way and comparing it to the one obtained via `GetHash()` is. I'm guessing you're trying to make the point that the txids are actually what you are claiming them to be (as opposed to just writing them in a comment), so the reader can manually check the difference between internal and human-readable lexicographic ordering. Is that really necessary?