💬 fjahr commented on pull request "assumeutxo: Drop block height from metadata":
(https://github.com/bitcoin/bitcoin/pull/30598#issuecomment-2275718173)
I have closed #30516 so we can move forward with the approach here. Looking forward to get reviews, thanks!
(https://github.com/bitcoin/bitcoin/pull/30598#issuecomment-2275718173)
I have closed #30516 so we can move forward with the approach here. Looking forward to get reviews, thanks!
💬 ismaelsadeeq commented on pull request "Assumeutxo: Sanitize block height in metadata":
(https://github.com/bitcoin/bitcoin/pull/30516#discussion_r1709407366)
@fjahr, I reviewed the code primarily to address PRs targeting issues in the v28.0 milestone. I haven’t gone through all the conceptual discussions here, as I’m still getting up to speed with the direction of the assumeutxo project.
(https://github.com/bitcoin/bitcoin/pull/30516#discussion_r1709407366)
@fjahr, I reviewed the code primarily to address PRs targeting issues in the v28.0 milestone. I haven’t gone through all the conceptual discussions here, as I’m still getting up to speed with the direction of the assumeutxo project.
💬 hodlinator commented on pull request "node: reduce unsafe uint256S usage":
(https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1709416606)
I agree taking `input` as `std::string` works better with the return type.
Instead of introducing a 2nd string at the end, one could simply:
```C++
input.insert(0, final_size - input.size(), '0');
return input;
```
(https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1709416606)
I agree taking `input` as `std::string` works better with the return type.
Instead of introducing a 2nd string at the end, one could simply:
```C++
input.insert(0, final_size - input.size(), '0');
return input;
```
💬 hodlinator commented on pull request "node: reduce unsafe uint256S usage":
(https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1709439226)
Using assertions to prevent the process from continuing in an unsupported state is certainly better than nothing at all.
Once the risk of unsupported state due to hardware failure or environment variable misconfiguration becomes known, one should implement non-assert error handling code, as the unsupported state is now expected in some circumstances.
I concede that in this case the environment variable is usually supposed to be set by Python functional tests, so there is a weak case to be
...
(https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1709439226)
Using assertions to prevent the process from continuing in an unsupported state is certainly better than nothing at all.
Once the risk of unsupported state due to hardware failure or environment variable misconfiguration becomes known, one should implement non-assert error handling code, as the unsupported state is now expected in some circumstances.
I concede that in this case the environment variable is usually supposed to be set by Python functional tests, so there is a weak case to be
...
💬 maflcko commented on pull request "node: reduce unsafe uint256S usage":
(https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1709471024)
> in this case the environment variable is usually supposed to be set by Python functional tests, so there is a weak case to be made that it is a logic error.
Going back to this case, I don't think `RANDOM_CTX_SEED` is set by python at all. It should only be set by a dev (or user), or not at all.
At least when I locally call `git grep RANDOM_CTX_SEED`, I don't see it.
(https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1709471024)
> in this case the environment variable is usually supposed to be set by Python functional tests, so there is a weak case to be made that it is a logic error.
Going back to this case, I don't think `RANDOM_CTX_SEED` is set by python at all. It should only be set by a dev (or user), or not at all.
At least when I locally call `git grep RANDOM_CTX_SEED`, I don't see it.
💬 hodlinator commented on pull request "node: reduce unsafe uint256S usage":
(https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1709484770)
Thanks for calling out my assumption that `test_runner.py --randomseed=X` could trickle down into unit tests.
Makes my case for it not being a logic error slightly stronger.
(https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1709484770)
Thanks for calling out my assumption that `test_runner.py --randomseed=X` could trickle down into unit tests.
Makes my case for it not being a logic error slightly stronger.
👍 ismaelsadeeq approved a pull request: "assumeutxo: Drop block height from metadata"
(https://github.com/bitcoin/bitcoin/pull/30598#pullrequestreview-2227841391)
Tested ACK 6a020f019f709b595b68516bb3772b811a962e7c
(https://github.com/bitcoin/bitcoin/pull/30598#pullrequestreview-2227841391)
Tested ACK 6a020f019f709b595b68516bb3772b811a962e7c
💬 ismaelsadeeq commented on pull request "assumeutxo: Drop block height from metadata":
(https://github.com/bitcoin/bitcoin/pull/30598#discussion_r1709476844)
nit: could indicate in the commit message `ShapShotMetadata` version was also bumped.
(https://github.com/bitcoin/bitcoin/pull/30598#discussion_r1709476844)
nit: could indicate in the commit message `ShapShotMetadata` version was also bumped.
💬 maflcko commented on pull request "feefrac: add support for evaluating at given size":
(https://github.com/bitcoin/bitcoin/pull/30535#discussion_r1709495436)
Can you explain why you remove this line of code? IIUC, this means that `!=` with `uint64_t` will now go through the implicit constructor call, and then through the new `operator<=>`.
If it is intentional, then removing operator `==` with `uint64_t` should be done as well for the same reason? If not, it may be better to keep them?
Mostly a consistency style-question, feel free to ignore.
(https://github.com/bitcoin/bitcoin/pull/30535#discussion_r1709495436)
Can you explain why you remove this line of code? IIUC, this means that `!=` with `uint64_t` will now go through the implicit constructor call, and then through the new `operator<=>`.
If it is intentional, then removing operator `==` with `uint64_t` should be done as well for the same reason? If not, it may be better to keep them?
Mostly a consistency style-question, feel free to ignore.
💬 paplorinc commented on pull request "node: reduce unsafe uint256S usage":
(https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1709502254)
> input.insert(0, final_size - input.size(), '0');
return input;
my functional programming past screams at the though of mutating parameters and also returning them
<insert meme with PTSD Chihuahua>
(https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1709502254)
> input.insert(0, final_size - input.size(), '0');
return input;
my functional programming past screams at the though of mutating parameters and also returning them
<insert meme with PTSD Chihuahua>
💬 hodlinator commented on pull request "node: reduce unsafe uint256S usage":
(https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1709508509)
Yeah, I had a bit of an itch as I was writing `return input`, but you made your bed when you started taking `std::string` by value. :)
(https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1709508509)
Yeah, I had a bit of an itch as I was writing `return input`, but you made your bed when you started taking `std::string` by value. :)
💬 Sjors commented on pull request "Stratum v2 Noise Protocol":
(https://github.com/bitcoin/bitcoin/pull/29346#discussion_r1709509545)
Update: `bip324.h` also uses `std::array`
(https://github.com/bitcoin/bitcoin/pull/29346#discussion_r1709509545)
Update: `bip324.h` also uses `std::array`
💬 stickies-v commented on pull request "node: reduce unsafe uint256S usage":
(https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1709526856)
> the performance of the method seems to be important
How so? There is one callsite, which can be called at most once, to parse one optional debug-only option.
> Could we either return an std::optional<std::string_view>
How would that work? `std::string_view` is a non-owning view.
> we're always copying the result before returning
In 802374b4355bd1dec7a88bba6287c55f935699fe, we're always allocating exactly once. In your example, we'd be allocating at least once, and twice if we ne
...
(https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1709526856)
> the performance of the method seems to be important
How so? There is one callsite, which can be called at most once, to parse one optional debug-only option.
> Could we either return an std::optional<std::string_view>
How would that work? `std::string_view` is a non-owning view.
> we're always copying the result before returning
In 802374b4355bd1dec7a88bba6287c55f935699fe, we're always allocating exactly once. In your example, we'd be allocating at least once, and twice if we ne
...
💬 hodlinator commented on pull request "node: reduce unsafe uint256S usage":
(https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1709538299)
```C++
input = util::RemovePrefixView(input, "0x");
```
This line gives me the heebie-jeebies if input is `std::string`. It might be okay as we are assigning a shorter string, but worst case, the assignment operator could free it's prior heap buffer and allocate a shorter buffer, before attempting to copy from the string_view which now might reference the deallocated buffer. Using `RemovePrefix()` should alleviate the problem.
(https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1709538299)
```C++
input = util::RemovePrefixView(input, "0x");
```
This line gives me the heebie-jeebies if input is `std::string`. It might be okay as we are assigning a shorter string, but worst case, the assignment operator could free it's prior heap buffer and allocate a shorter buffer, before attempting to copy from the string_view which now might reference the deallocated buffer. Using `RemovePrefix()` should alleviate the problem.
💬 glozow commented on pull request "feefrac: add support for evaluating at given size":
(https://github.com/bitcoin/bitcoin/pull/30535#discussion_r1709543030)
Question: why did you make this a templated function instead of adding a bool parameter? Is it a compile-time optimization?
(https://github.com/bitcoin/bitcoin/pull/30535#discussion_r1709543030)
Question: why did you make this a templated function instead of adding a bool parameter? Is it a compile-time optimization?
💬 furszy commented on issue "ci: failure in `wallet_multiwallet.py --legacy-wallet` - (`void wallet::UnloadWallet(std::shared_ptr<CWallet> &&): Assertion 'it.second' failed.`)":
(https://github.com/bitcoin/bitcoin/issues/29073#issuecomment-2275892677)
> But the "unloadwallet RPC call at the same time as a shutdown" case I mentioned can happen because UnloadWallets which runs on shutdown makes a copy of the wallets vector, so if an unloadwallet RPC call happens at the same time as that they can both call UnloadWallet on the same wallet.
I agree that there is an issue there, but how do you link it to this failure? No shutdown should be executed along the "Concurrent wallet loading" test case. The scenario merely involves two or more threads
...
(https://github.com/bitcoin/bitcoin/issues/29073#issuecomment-2275892677)
> But the "unloadwallet RPC call at the same time as a shutdown" case I mentioned can happen because UnloadWallets which runs on shutdown makes a copy of the wallets vector, so if an unloadwallet RPC call happens at the same time as that they can both call UnloadWallet on the same wallet.
I agree that there is an issue there, but how do you link it to this failure? No shutdown should be executed along the "Concurrent wallet loading" test case. The scenario merely involves two or more threads
...
💬 sipa commented on pull request "feefrac: add support for evaluating at given size":
(https://github.com/bitcoin/bitcoin/pull/30535#discussion_r1709562802)
Yeah, exactly. I think we want this function to be fast (and the rounding mode is always known at compile time anyway), while the `Div` functions are only needed in exceptional cases (negative fees, or >20 BTC fees) anyway.
(https://github.com/bitcoin/bitcoin/pull/30535#discussion_r1709562802)
Yeah, exactly. I think we want this function to be fast (and the rounding mode is always known at compile time anyway), while the `Div` functions are only needed in exceptional cases (negative fees, or >20 BTC fees) anyway.
💬 glozow commented on pull request "feefrac: add support for evaluating at given size":
(https://github.com/bitcoin/bitcoin/pull/30535#discussion_r1709566877)
Makes sense!
(https://github.com/bitcoin/bitcoin/pull/30535#discussion_r1709566877)
Makes sense!
💬 stickies-v commented on pull request "node: reduce unsafe uint256S usage":
(https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1709568559)
Force pushed to use `std::cerr` since it seems everyone can get behind that, thanks for the suggestion.
(https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1709568559)
Force pushed to use `std::cerr` since it seems everyone can get behind that, thanks for the suggestion.
💬 hebasto commented on pull request "build: Introduce CMake-based build system":
(https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1709569185)
I've tested on macOS Monterey 12.7.6 (Intel) with Homebrew's `llvm@16`. Everything works flawlessly.
> Is there any way I could help here?
Mind providing full _verbose_ build logs for both Autotools and CMake please?
(https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1709569185)
I've tested on macOS Monterey 12.7.6 (Intel) with Homebrew's `llvm@16`. Everything works flawlessly.
> Is there any way I could help here?
Mind providing full _verbose_ build logs for both Autotools and CMake please?