💬 maflcko commented on pull request "refactor: Replace g_genesis_wait_cv with m_tip_block_cv":
(https://github.com/bitcoin/bitcoin/pull/30967#discussion_r1776550909)
> The Sv2 TemplateProvider unit tests rely on `SetMockTime`
mocktime only affects the `NodeClock`, neither the system clock, nor the steady clock (obviously), so I don't think this will affect your unit tests and you don't have to change anything.
(https://github.com/bitcoin/bitcoin/pull/30967#discussion_r1776550909)
> The Sv2 TemplateProvider unit tests rely on `SetMockTime`
mocktime only affects the `NodeClock`, neither the system clock, nor the steady clock (obviously), so I don't think this will affect your unit tests and you don't have to change anything.
💬 Sjors commented on pull request "Mining interface: getCoinbaseMerklePath() and submitSolution()":
(https://github.com/bitcoin/bitcoin/pull/30955#issuecomment-2376225530)
Rebased after #30510.
(https://github.com/bitcoin/bitcoin/pull/30955#issuecomment-2376225530)
Rebased after #30510.
💬 fanquake commented on issue "assumeutxo: not syncing the snapshot chainstate":
(https://github.com/bitcoin/bitcoin/issues/30971#issuecomment-2376227995)
> Are you syncing from random peers or do you have some local setup?
This was syncing from random peers.
(https://github.com/bitcoin/bitcoin/issues/30971#issuecomment-2376227995)
> Are you syncing from random peers or do you have some local setup?
This was syncing from random peers.
💬 Sjors commented on pull request "rpc: add optional blockhash to waitfornewblock":
(https://github.com/bitcoin/bitcoin/pull/30635#issuecomment-2376260403)
Now that #30409 landed this is a single commit, and ready for review.
(https://github.com/bitcoin/bitcoin/pull/30635#issuecomment-2376260403)
Now that #30409 landed this is a single commit, and ready for review.
👋 Sjors's pull request is ready for review: "rpc: add optional blockhash to waitfornewblock"
(https://github.com/bitcoin/bitcoin/pull/30635)
(https://github.com/bitcoin/bitcoin/pull/30635)
👍 itornaza approved a pull request: "Mining interface: getCoinbaseMerklePath() and submitSolution()"
(https://github.com/bitcoin/bitcoin/pull/30955#pullrequestreview-2330484303)
Code review ACK 525e9dcba0b8c6744bcd3725864f39786afc8ed5
In order to verify that `MerkleComputation()`, `ComputeMerkleBranch()`, `BlockMerkleBranch()` were copied verbatim from their resting place at `src/test/merkle_tests.cpp` to their new home `src/consensus/merkle.cpp`, I copied the two versions of the functions in separate files and compared them with `diff`. The only difference I noticed is that now the function `BlockMerkleBranch()` is not declared as `static` which is correct, because
...
(https://github.com/bitcoin/bitcoin/pull/30955#pullrequestreview-2330484303)
Code review ACK 525e9dcba0b8c6744bcd3725864f39786afc8ed5
In order to verify that `MerkleComputation()`, `ComputeMerkleBranch()`, `BlockMerkleBranch()` were copied verbatim from their resting place at `src/test/merkle_tests.cpp` to their new home `src/consensus/merkle.cpp`, I copied the two versions of the functions in separate files and compared them with `diff`. The only difference I noticed is that now the function `BlockMerkleBranch()` is not declared as `static` which is correct, because
...
💬 Sjors commented on pull request "Mining interface: getCoinbaseMerklePath() and submitSolution()":
(https://github.com/bitcoin/bitcoin/pull/30955#issuecomment-2376282083)
@itornaza you may also find `git diff --color-moved=dimmed-zebra` useful.
(https://github.com/bitcoin/bitcoin/pull/30955#issuecomment-2376282083)
@itornaza you may also find `git diff --color-moved=dimmed-zebra` useful.
📝 fanquake opened a pull request: "doc: fix `loadtxoutset` example"
(https://github.com/bitcoin/bitcoin/pull/30973)
The current order is incorrect:
```bash
./build/src/bitcoin-cli loadtxoutset -rpcclienttimeout=0 utxo-840000.dat
error code: -1
error message:
loadtxoutset "path"
```
(https://github.com/bitcoin/bitcoin/pull/30973)
The current order is incorrect:
```bash
./build/src/bitcoin-cli loadtxoutset -rpcclienttimeout=0 utxo-840000.dat
error code: -1
error message:
loadtxoutset "path"
```
💬 maflcko commented on pull request "init: Remove retry for loop":
(https://github.com/bitcoin/bitcoin/pull/30968#discussion_r1776654445)
> clang-format has a nice "BinPack" and "Align" options
Last time I checked they wouldn't take any effect with `ColumnLimit: 0`, but maybe I am wrong, or this may have changed. However, once there is a line break, I think using a line break for all is cleaner (`cargo fmt` agrees with me and consistently either uses a line-separator or space-separator, but never mixes them [1]). It would be nice if there was a clang-format config that did what `cargo fmt` does by default.
[1] https://
...
(https://github.com/bitcoin/bitcoin/pull/30968#discussion_r1776654445)
> clang-format has a nice "BinPack" and "Align" options
Last time I checked they wouldn't take any effect with `ColumnLimit: 0`, but maybe I am wrong, or this may have changed. However, once there is a line break, I think using a line break for all is cleaner (`cargo fmt` agrees with me and consistently either uses a line-separator or space-separator, but never mixes them [1]). It would be nice if there was a clang-format config that did what `cargo fmt` does by default.
[1] https://
...
💬 maflcko commented on pull request "init: Remove retry for loop":
(https://github.com/bitcoin/bitcoin/pull/30968#issuecomment-2376345627)
Reviewed, but did not test the retry flow in the GUI
review ACK e9d60af9889c12b4d427adefa53fd234e417f8f6 🚸
<details><summary>Show signature</summary>
Signature:
```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpw
...
(https://github.com/bitcoin/bitcoin/pull/30968#issuecomment-2376345627)
Reviewed, but did not test the retry flow in the GUI
review ACK e9d60af9889c12b4d427adefa53fd234e417f8f6 🚸
<details><summary>Show signature</summary>
Signature:
```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpw
...
🤔 josibake reviewed a pull request: "kernel: Move block tree db open to block manager"
(https://github.com/bitcoin/bitcoin/pull/30965#pullrequestreview-2330478429)
Concept ACK
Conceptually, this makes a lot more sense and makes the code much easier to understand, IMO. Left some comments while doing an initial pass, more questions than concrete suggestions. I did find it a bit hard to parse what was going with a lot of the code being move-only, but from across different files. Not sure if much can be done here, but it would make this easier to follow if you could separate as much of the move-only code changes into their own commit(s)? Specifically, I'm t
...
(https://github.com/bitcoin/bitcoin/pull/30965#pullrequestreview-2330478429)
Concept ACK
Conceptually, this makes a lot more sense and makes the code much easier to understand, IMO. Left some comments while doing an initial pass, more questions than concrete suggestions. I did find it a bit hard to parse what was going with a lot of the code being move-only, but from across different files. Not sure if much can be done here, but it would make this easier to follow if you could separate as much of the move-only code changes into their own commit(s)? Specifically, I'm t
...
💬 josibake commented on pull request "kernel: Move block tree db open to block manager":
(https://github.com/bitcoin/bitcoin/pull/30965#discussion_r1776635876)
Assuming this is just a comment explaining what was already happening and documenting a behaviour change introduced in this PR? (note to self to test this).
(https://github.com/bitcoin/bitcoin/pull/30965#discussion_r1776635876)
Assuming this is just a comment explaining what was already happening and documenting a behaviour change introduced in this PR? (note to self to test this).
💬 josibake commented on pull request "kernel: Move block tree db open to block manager":
(https://github.com/bitcoin/bitcoin/pull/30965#discussion_r1776630244)
Might be premature at this stage, but in theory we could be opening both the block database and txdb at this stage. I haven't looked at `dbwrapper_error` yet so not sure if this is possible, but perhaps we could bubble up the actual db name that failed to open instead of assuming it's always the block database?
(https://github.com/bitcoin/bitcoin/pull/30965#discussion_r1776630244)
Might be premature at this stage, but in theory we could be opening both the block database and txdb at this stage. I haven't looked at `dbwrapper_error` yet so not sure if this is possible, but perhaps we could bubble up the actual db name that failed to open instead of assuming it's always the block database?
💬 josibake commented on pull request "kernel: Move block tree db open to block manager":
(https://github.com/bitcoin/bitcoin/pull/30965#discussion_r1776602376)
I'm guessing you moved the values up from below (L109-L112) so that you could set the value once and use `cache_sizes.block_tree_db` to set `block_tree_db_cache_size` on L127? Otherwise it's not clear to me why those values were moved.
(https://github.com/bitcoin/bitcoin/pull/30965#discussion_r1776602376)
I'm guessing you moved the values up from below (L109-L112) so that you could set the value once and use `cache_sizes.block_tree_db` to set `block_tree_db_cache_size` on L127? Otherwise it's not clear to me why those values were moved.
💬 josibake commented on pull request "kernel: Move block tree db open to block manager":
(https://github.com/bitcoin/bitcoin/pull/30965#discussion_r1776649353)
This code feels a bit smelly, so nice to see you are dropping it by moving the database initialisation into the `BlockManager`. Will need to look at the old code more closely to understand what "previous loop" this comment is referring to.
(https://github.com/bitcoin/bitcoin/pull/30965#discussion_r1776649353)
This code feels a bit smelly, so nice to see you are dropping it by moving the database initialisation into the `BlockManager`. Will need to look at the old code more closely to understand what "previous loop" this comment is referring to.
💬 josibake commented on pull request "kernel: Move block tree db open to block manager":
(https://github.com/bitcoin/bitcoin/pull/30965#discussion_r1776638088)
Nice! Makes a lot more sense to me to have block tree database options in the `BlockManagerOptions`
(https://github.com/bitcoin/bitcoin/pull/30965#discussion_r1776638088)
Nice! Makes a lot more sense to me to have block tree database options in the `BlockManagerOptions`
💬 josibake commented on pull request "kernel: Move block tree db open to block manager":
(https://github.com/bitcoin/bitcoin/pull/30965#discussion_r1776620730)
Note to self (and other reviewers): this move wasn't immediately obvious to me, until I noticed `CacheSizes` is set on L1521, and then the value `cache_sizes.block_tree_db` is used when creating the `BlockManager::Options`. As a nice side effect, it seems better to have the logs for cache configuration first, and then proceed with setting and creating the `BlockManager`.
(https://github.com/bitcoin/bitcoin/pull/30965#discussion_r1776620730)
Note to self (and other reviewers): this move wasn't immediately obvious to me, until I noticed `CacheSizes` is set on L1521, and then the value `cache_sizes.block_tree_db` is used when creating the `BlockManager::Options`. As a nice side effect, it seems better to have the logs for cache configuration first, and then proceed with setting and creating the `BlockManager`.
💬 josibake commented on pull request "kernel: Move block tree db open to block manager":
(https://github.com/bitcoin/bitcoin/pull/30965#discussion_r1776651167)
Nevermind, I can see this a move only from `src/chainstate.h`
(https://github.com/bitcoin/bitcoin/pull/30965#discussion_r1776651167)
Nevermind, I can see this a move only from `src/chainstate.h`
💬 maflcko commented on pull request "doc: fix `loadtxoutset` example":
(https://github.com/bitcoin/bitcoin/pull/30973#issuecomment-2376362674)
review ACK 286725168ae0309e427b1204a0724a4ba7cbe860
(https://github.com/bitcoin/bitcoin/pull/30973#issuecomment-2376362674)
review ACK 286725168ae0309e427b1204a0724a4ba7cbe860
🚀 fanquake merged a pull request: "ci: add `LLVM_SYMBOLIZER_PATH` to Valgrind fuzz job"
(https://github.com/bitcoin/bitcoin/pull/30961)
(https://github.com/bitcoin/bitcoin/pull/30961)