💬 fjahr commented on pull request "assumeutxo: Add dumptxoutset height param, remove shell scripts":
(https://github.com/bitcoin/bitcoin/pull/29553#discussion_r1719045686)
I made some edits here, making sure to clearly spell out what you suggested to include.
(https://github.com/bitcoin/bitcoin/pull/29553#discussion_r1719045686)
I made some edits here, making sure to clearly spell out what you suggested to include.
💬 fjahr commented on pull request "assumeutxo: Add dumptxoutset height param, remove shell scripts":
(https://github.com/bitcoin/bitcoin/pull/29553#issuecomment-2292338592)
Addressed feedback and rebased.
There is a bit of a hack in the tests now that is made necessary by an unrelated bug: #26245 I marked this with an TODO accordingly.
(https://github.com/bitcoin/bitcoin/pull/29553#issuecomment-2292338592)
Addressed feedback and rebased.
There is a bit of a hack in the tests now that is made necessary by an unrelated bug: #26245 I marked this with an TODO accordingly.
💬 hodlinator commented on pull request "refactor: Replace ParseHex with consteval ArrayFromHex":
(https://github.com/bitcoin/bitcoin/pull/30377#issuecomment-2292367131)
Thanks for taking the time @ryanofsky!
Your troubles with runtime vs compile time memory is indeed part of what fed into prior solutions. Cool that `VectorFromHex<"1234">()` works, but I'm happy you're not a fan either.
While I did like the symmetry of `ArrayFromHex` with the recently added `Txid::FromHex` and `uint256::FromHex`, the latter ones like to reverse the byte order though, and `HexLiteral` is 2 chars less. `HexLiteral` feels more like prime real estate, so hiding behind the fig-
...
(https://github.com/bitcoin/bitcoin/pull/30377#issuecomment-2292367131)
Thanks for taking the time @ryanofsky!
Your troubles with runtime vs compile time memory is indeed part of what fed into prior solutions. Cool that `VectorFromHex<"1234">()` works, but I'm happy you're not a fan either.
While I did like the symmetry of `ArrayFromHex` with the recently added `Txid::FromHex` and `uint256::FromHex`, the latter ones like to reverse the byte order though, and `HexLiteral` is 2 chars less. `HexLiteral` feels more like prime real estate, so hiding behind the fig-
...
💬 hodlinator commented on pull request "refactor: Replace ParseHex with consteval ArrayFromHex":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1719074063)
Thank you for being persistent! Must have had other local changes interacting badly with it when I tried it. Got it working with only your diff, so if we end up resurrecting `ConstevalHexLiteral` I'll go with your version.
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1719074063)
Thank you for being persistent! Must have had other local changes interacting badly with it when I tried it. Got it working with only your diff, so if we end up resurrecting `ConstevalHexLiteral` I'll go with your version.
💬 ryanofsky commented on pull request "assumeutxo: Add dumptxoutset height param, remove shell scripts":
(https://github.com/bitcoin/bitcoin/pull/29553#discussion_r1719054774)
In commit "rpc, refactor: Prevent potential race conditions in dumptxoutset" (0c1238917a5edf6bdcae15aab5f91b1bdd2540b9)
Would be good to update comment on line 2852. Could replace "use below this block" with "use in WriteUTXOSnapshot"
(https://github.com/bitcoin/bitcoin/pull/29553#discussion_r1719054774)
In commit "rpc, refactor: Prevent potential race conditions in dumptxoutset" (0c1238917a5edf6bdcae15aab5f91b1bdd2540b9)
Would be good to update comment on line 2852. Could replace "use below this block" with "use in WriteUTXOSnapshot"
👍 ryanofsky approved a pull request: "assumeutxo: Add dumptxoutset height param, remove shell scripts"
(https://github.com/bitcoin/bitcoin/pull/29553#pullrequestreview-2241421928)
Code review ACK 0c1238917a5edf6bdcae15aab5f91b1bdd2540b9. Main change since last review is that dumptxoutset will no longer dump a snapshot that can't be loaded by default or roll back the chain by default. Instead you need to specify "latest" or "rollback" to choose the behavior you want, or specify the height or hash of a specific block.
It is now possible to specify a snapshot block hash instead of a just height, and there are test and documentation improvements, and a new commit that lock
...
(https://github.com/bitcoin/bitcoin/pull/29553#pullrequestreview-2241421928)
Code review ACK 0c1238917a5edf6bdcae15aab5f91b1bdd2540b9. Main change since last review is that dumptxoutset will no longer dump a snapshot that can't be loaded by default or roll back the chain by default. Instead you need to specify "latest" or "rollback" to choose the behavior you want, or specify the height or hash of a specific block.
It is now possible to specify a snapshot block hash instead of a just height, and there are test and documentation improvements, and a new commit that lock
...
💬 ryanofsky commented on pull request "assumeutxo: Add dumptxoutset height param, remove shell scripts":
(https://github.com/bitcoin/bitcoin/pull/29553#discussion_r1719053204)
In commit "rpc, refactor: Prevent potential race conditions in dumptxoutset" (0c1238917a5edf6bdcae15aab5f91b1bdd2540b9)
Would be nice to add a comment explaining locking here. Maybe "// Lock the chainstate before calling PrepareUtxoSnapshot, to be able to get a UTXO database cursor while the chain is pointing at the target block. After that, release the lock while calling WriteUTXOSnapshot. The cursor will remain valid and be used by WriteUTXOSnapshot to write a consistent snapshot even if th
...
(https://github.com/bitcoin/bitcoin/pull/29553#discussion_r1719053204)
In commit "rpc, refactor: Prevent potential race conditions in dumptxoutset" (0c1238917a5edf6bdcae15aab5f91b1bdd2540b9)
Would be nice to add a comment explaining locking here. Maybe "// Lock the chainstate before calling PrepareUtxoSnapshot, to be able to get a UTXO database cursor while the chain is pointing at the target block. After that, release the lock while calling WriteUTXOSnapshot. The cursor will remain valid and be used by WriteUTXOSnapshot to write a consistent snapshot even if th
...
💬 ryanofsky commented on pull request "assumeutxo: Add dumptxoutset height param, remove shell scripts":
(https://github.com/bitcoin/bitcoin/pull/29553#discussion_r1719061724)
In commit "rpc, refactor: Prevent potential race conditions in dumptxoutset" (0c1238917a5edf6bdcae15aab5f91b1bdd2540b9)
Would it make any sense to call `ReconsiderBlock()` and `disable_network.reset()` before calling `WriteUTXOSnapshot` instead of after?
Not sure if that might make the node more usable while the snapshot is being created, or just slow down writing the snapshot and increase resource usage.
Change wouldn't be appropriate in this commit anyway, but could be a possible foll
...
(https://github.com/bitcoin/bitcoin/pull/29553#discussion_r1719061724)
In commit "rpc, refactor: Prevent potential race conditions in dumptxoutset" (0c1238917a5edf6bdcae15aab5f91b1bdd2540b9)
Would it make any sense to call `ReconsiderBlock()` and `disable_network.reset()` before calling `WriteUTXOSnapshot` instead of after?
Not sure if that might make the node more usable while the snapshot is being created, or just slow down writing the snapshot and increase resource usage.
Change wouldn't be appropriate in this commit anyway, but could be a possible foll
...
👍 ryanofsky approved a pull request: "refactor: Replace ParseHex with consteval HexLiteral"
(https://github.com/bitcoin/bitcoin/pull/30377#pullrequestreview-2241499736)
Code review ACK e4032e253c2dfc8d75defd450dbb23ccf689c390 if visual studio and clang-tidy errors are fixed
https://cirrus-ci.com/task/5821174167371776
https://github.com/bitcoin/bitcoin/actions/runs/10411271977/job/28834888600?pr=30377#step:20:2031
Thanks for updating. To be clear I don't have really have any issue with embedding hex constants instead byte constants in the compiled code. I mainly just thought behavior of VectorFromHex function was hard to explain and confusing, arbitrarily
...
(https://github.com/bitcoin/bitcoin/pull/30377#pullrequestreview-2241499736)
Code review ACK e4032e253c2dfc8d75defd450dbb23ccf689c390 if visual studio and clang-tidy errors are fixed
https://cirrus-ci.com/task/5821174167371776
https://github.com/bitcoin/bitcoin/actions/runs/10411271977/job/28834888600?pr=30377#step:20:2031
Thanks for updating. To be clear I don't have really have any issue with embedding hex constants instead byte constants in the compiled code. I mainly just thought behavior of VectorFromHex function was hard to explain and confusing, arbitrarily
...
💬 ryanofsky commented on pull request "refactor: Replace ParseHex with consteval HexLiteral":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1719107583)
In commit "refactor: add util::HexLiteral and util::Vec using statements" (ced7cec1c6d79159f8212d3a92a8f7583ef11884)
It seems like there is a problem with this on windows, looks like because begin and end are returning `std::_Array_const_iterator` types, instead of character pointers. Following change might fix it:
```diff
- return {array.begin(), array.end()};
+ return {array.data(), array.data() + array.size()};
```
Error is:
https://github.com/bitcoin/bitcoin/actions/runs/1
...
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1719107583)
In commit "refactor: add util::HexLiteral and util::Vec using statements" (ced7cec1c6d79159f8212d3a92a8f7583ef11884)
It seems like there is a problem with this on windows, looks like because begin and end are returning `std::_Array_const_iterator` types, instead of character pointers. Following change might fix it:
```diff
- return {array.begin(), array.end()};
+ return {array.data(), array.data() + array.size()};
```
Error is:
https://github.com/bitcoin/bitcoin/actions/runs/1
...
💬 fjahr commented on pull request "test: assumeutxo: add missing tests in wallet_assumeutxo.py":
(https://github.com/bitcoin/bitcoin/pull/30455#discussion_r1719107427)
nit: You could move this to the top of this block like with the other cases (unless you duplicate anyway)
(https://github.com/bitcoin/bitcoin/pull/30455#discussion_r1719107427)
nit: You could move this to the top of this block like with the other cases (unless you duplicate anyway)
💬 fjahr commented on pull request "test: assumeutxo: add missing tests in wallet_assumeutxo.py":
(https://github.com/bitcoin/bitcoin/pull/30455#discussion_r1719109497)
nit: The three lines are similar to above for n1, so could be put into a function together
(https://github.com/bitcoin/bitcoin/pull/30455#discussion_r1719109497)
nit: The three lines are similar to above for n1, so could be put into a function together
💬 fjahr commented on pull request "test: assumeutxo: add missing tests in wallet_assumeutxo.py":
(https://github.com/bitcoin/bitcoin/pull/30455#discussion_r1719103180)
You can simplify this by just returning the result from the function and then doing these checks respectively in the two places where the function is used now. Should drop the `test_` from the name as well then.
(https://github.com/bitcoin/bitcoin/pull/30455#discussion_r1719103180)
You can simplify this by just returning the result from the function and then doing these checks respectively in the two places where the function is used now. Should drop the `test_` from the name as well then.
💬 fjahr commented on pull request "test: assumeutxo: add missing tests in wallet_assumeutxo.py":
(https://github.com/bitcoin/bitcoin/pull/30455#discussion_r1719128993)
Sorry for the long wait, I finally got around to look at your branch @alfonsoromanz . You are looking at this `wallet_last_processed_block` but that is coming out of node 0. When you load the wallet into node 2 it doesn't know anything about it, so this can be ignored. What you might be thinking of instead is the birthdate of the wallet, that's part of the backup afaict so that should be relevant. But the birthdate is earlier than 299 because the wallet seems to be created before a bunch of bloc
...
(https://github.com/bitcoin/bitcoin/pull/30455#discussion_r1719128993)
Sorry for the long wait, I finally got around to look at your branch @alfonsoromanz . You are looking at this `wallet_last_processed_block` but that is coming out of node 0. When you load the wallet into node 2 it doesn't know anything about it, so this can be ignored. What you might be thinking of instead is the birthdate of the wallet, that's part of the backup afaict so that should be relevant. But the birthdate is earlier than 299 because the wallet seems to be created before a bunch of bloc
...
💬 tdb3 commented on pull request "build: Introduce CMake-based build system":
(https://github.com/bitcoin/bitcoin/pull/30454#issuecomment-2292561601)
Built and ran tests successfully with 67b1e236334f38ec4e4d2251dbdfb790f20ed88b
More details:
Debian 12.6
cmake version 3.25.1
gcc/g++ (Debian 12.2.0-14) 12.2.0
```
cmake -B build
cmake --build build -j18
ctest --test-dir build
```
(https://github.com/bitcoin/bitcoin/pull/30454#issuecomment-2292561601)
Built and ran tests successfully with 67b1e236334f38ec4e4d2251dbdfb790f20ed88b
More details:
Debian 12.6
cmake version 3.25.1
gcc/g++ (Debian 12.2.0-14) 12.2.0
```
cmake -B build
cmake --build build -j18
ctest --test-dir build
```
💬 maflcko commented on pull request "refactor: Replace ParseHex with consteval HexLiteral":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1719324036)
Is https://github.com/bitcoin/bitcoin/pull/29369 related or possibly a fix?
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1719324036)
Is https://github.com/bitcoin/bitcoin/pull/29369 related or possibly a fix?
💬 maflcko commented on pull request "refactor: Replace ParseHex with consteval HexLiteral":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1719345193)
style nit in 84c830f27fd62db4a9cb93bf6d28a86f7751e504: Could use `std::byte` as default? (See below for reasoning)
(Reply to https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1719000295)
I think your link should say https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1716903532
This is a newly introduced function, so I think changing it should not extend to untouched files.
To clarify, I am not saying that you should switch callers to use `std::byte`, only the default.
...
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1719345193)
style nit in 84c830f27fd62db4a9cb93bf6d28a86f7751e504: Could use `std::byte` as default? (See below for reasoning)
(Reply to https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1719000295)
I think your link should say https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1716903532
This is a newly introduced function, so I think changing it should not extend to untouched files.
To clarify, I am not saying that you should switch callers to use `std::byte`, only the default.
...
💬 maflcko commented on pull request "refactor: Replace ParseHex with consteval HexLiteral":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1719367069)
> It's because part of my goal with this PR was to etch these bytes into the runtime binary. Things that can be done at compile time without too much hassle should be done at compile time IMO.
>
> If we go the `Vec(HexLiteral` route at least it becomes shorter.
Makes sense, thanks for looking into it!
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1719367069)
> It's because part of my goal with this PR was to etch these bytes into the runtime binary. Things that can be done at compile time without too much hassle should be done at compile time IMO.
>
> If we go the `Vec(HexLiteral` route at least it becomes shorter.
Makes sense, thanks for looking into it!
💬 maflcko commented on pull request "fuzz: Faster utxo_snapshot fuzz target":
(https://github.com/bitcoin/bitcoin/pull/30644#discussion_r1719382540)
Sure, fixed the style
(https://github.com/bitcoin/bitcoin/pull/30644#discussion_r1719382540)
Sure, fixed the style
💬 maflcko commented on pull request "fuzz: Faster utxo_snapshot fuzz target":
(https://github.com/bitcoin/bitcoin/pull/30644#discussion_r1719384643)
Sure, removed the ranges include.
(Maybe another reason to enforce iwyu in the CI)
(https://github.com/bitcoin/bitcoin/pull/30644#discussion_r1719384643)
Sure, removed the ranges include.
(Maybe another reason to enforce iwyu in the CI)