💬 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?
💬 stickies-v commented on pull request "node: reduce unsafe uint256S usage":
(https://github.com/bitcoin/bitcoin/pull/30569#issuecomment-2275904176)
Force pushed to [terminate with `std::abort()`](https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1709568559) in case of invalid `RANDOM_CTX_SEED` input.
(https://github.com/bitcoin/bitcoin/pull/30569#issuecomment-2275904176)
Force pushed to [terminate with `std::abort()`](https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1709568559) in case of invalid `RANDOM_CTX_SEED` input.
💬 instagibbs commented on pull request "cluster mempool: optimized candidate search":
(https://github.com/bitcoin/bitcoin/pull/30286#discussion_r1709581953)
LGTM
(https://github.com/bitcoin/bitcoin/pull/30286#discussion_r1709581953)
LGTM
💬 sipa commented on pull request "feefrac: add support for evaluating at given size":
(https://github.com/bitcoin/bitcoin/pull/30535#discussion_r1709591119)
I don't think so: https://godbolt.org/z/vYr4Tsbde
The implicitly-defined `operator!=` falls back to using `operator==`. Only the implicitly defined `operator<`, `operator>`, `operator<=`, `operator>=` falls back to using `operator<=>`.
(https://github.com/bitcoin/bitcoin/pull/30535#discussion_r1709591119)
I don't think so: https://godbolt.org/z/vYr4Tsbde
The implicitly-defined `operator!=` falls back to using `operator==`. Only the implicitly defined `operator<`, `operator>`, `operator<=`, `operator>=` falls back to using `operator<=>`.
💬 jonatack commented on pull request "net: Replace libnatpmp with built-in PCP+NATPMP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1709596999)
> Bitcoin Core had a strong preference to connect to peers that listen on port `8333`. Is this what you mean? That was removed at some point.
Yes, that preference for clearnet connections was removed in https://github.com/bitcoin/bitcoin/pull/23542.
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1709596999)
> Bitcoin Core had a strong preference to connect to peers that listen on port `8333`. Is this what you mean? That was removed at some point.
Yes, that preference for clearnet connections was removed in https://github.com/bitcoin/bitcoin/pull/23542.
💬 fjahr commented on pull request "net: Replace libnatpmp with built-in PCP+NATPMP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#issuecomment-2275938528)
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/30043#issuecomment-2275938528)
Concept ACK
💬 instagibbs commented on pull request "cluster mempool: optimized candidate search":
(https://github.com/bitcoin/bitcoin/pull/30286#discussion_r1709617445)
I think this has helped a lot, "clusterlin: avoid recomputing potential set on every split (optimization)" now makes sense to me. Lots of interlocking parts, so having small bite-sizes helps.
Can you motivate why `reconsider_pot` is set as true on inclusion of `anc` and not on exclude?
When `anc` is included, it's included in both `inc` and `pot`, then:
`if (reconsider_pot) consider_inc = pot.transactions - inc.transactions;`
so no part of `anc` will be in `consider_inc`.
(https://github.com/bitcoin/bitcoin/pull/30286#discussion_r1709617445)
I think this has helped a lot, "clusterlin: avoid recomputing potential set on every split (optimization)" now makes sense to me. Lots of interlocking parts, so having small bite-sizes helps.
Can you motivate why `reconsider_pot` is set as true on inclusion of `anc` and not on exclude?
When `anc` is included, it's included in both `inc` and `pot`, then:
`if (reconsider_pot) consider_inc = pot.transactions - inc.transactions;`
so no part of `anc` will be in `consider_inc`.
💬 paplorinc commented on pull request "node: reduce unsafe uint256S usage":
(https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1709618160)
Will let you decide, there are multiple ways to do this, each would be slightly better in a different scenario.
Whichever produces the cleanest code, choose that :)
(https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1709618160)
Will let you decide, there are multiple ways to do this, each would be slightly better in a different scenario.
Whichever produces the cleanest code, choose that :)