💬 hebasto commented on pull request "build: Introduce CMake-based build system":
(https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1709374557)
The quoting style was inherited from the master branch. Such an approach makes the diff easier to review.
(https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1709374557)
The quoting style was inherited from the master branch. Such an approach makes the diff easier to review.
💬 maflcko commented on pull request "node: reduce unsafe uint256S usage":
(https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1709378173)
> > _Therefore we will throw an assertion if the write fails._
>
> Not sure if the actual code really uses assertions to stop the process if the database reports write failure. Maybe Bitcoin Core has a looser policy than I'm used to, and at least @maflcko is okay with using it in tests.
I haven't checked the code, but "throw assertion" could mean "throw assertion error", which seems fine to use in this context the quote was taken from.
Not to be confused with other context, where abor
...
(https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1709378173)
> > _Therefore we will throw an assertion if the write fails._
>
> Not sure if the actual code really uses assertions to stop the process if the database reports write failure. Maybe Bitcoin Core has a looser policy than I'm used to, and at least @maflcko is okay with using it in tests.
I haven't checked the code, but "throw assertion" could mean "throw assertion error", which seems fine to use in this context the quote was taken from.
Not to be confused with other context, where abor
...
💬 maflcko commented on pull request "guix: bump time-machine to 7bf1d7aeaffba15c4f680f93ae88fbef25427252":
(https://github.com/bitcoin/bitcoin/pull/30609#issuecomment-2275700030)
Very nice.
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/30609#issuecomment-2275700030)
Very nice.
Concept ACK
💬 hebasto commented on pull request "build: Introduce CMake-based build system":
(https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1709388813)
Yes. The CMake [Regex Specification](https://cmake.org/cmake/help/latest/command/string.html#regex-specification) does not include `\w` support.
(https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1709388813)
Yes. The CMake [Regex Specification](https://cmake.org/cmake/help/latest/command/string.html#regex-specification) does not include `\w` support.
👍 ismaelsadeeq approved a pull request: "feefrac: add support for evaluating at given size"
(https://github.com/bitcoin/bitcoin/pull/30535#pullrequestreview-2227244550)
Code review ACK 9590cdd1afc78ff73fdccd2d016075bf40b6b0e2
> @ismaelsadeeq It's a possibility, if we'd want to replace CFeeRate entirely. Another possibility is keeping CFeeRate and its interface, but make it be an encapsulated FeeFrac object (that e.g. on serialization converts to sats/kvb, but that otherwise keeps exact fractions).
I prefer the idea of encapsulating `FeeFrac` within `CFeeRate` given how widely `CFeeRate` is relied upon. I’ve attempt to implement this approach on top of
...
(https://github.com/bitcoin/bitcoin/pull/30535#pullrequestreview-2227244550)
Code review ACK 9590cdd1afc78ff73fdccd2d016075bf40b6b0e2
> @ismaelsadeeq It's a possibility, if we'd want to replace CFeeRate entirely. Another possibility is keeping CFeeRate and its interface, but make it be an encapsulated FeeFrac object (that e.g. on serialization converts to sats/kvb, but that otherwise keeps exact fractions).
I prefer the idea of encapsulating `FeeFrac` within `CFeeRate` given how widely `CFeeRate` is relied upon. I’ve attempt to implement this approach on top of
...
💬 ismaelsadeeq commented on pull request "feefrac: add support for evaluating at given size":
(https://github.com/bitcoin/bitcoin/pull/30535#discussion_r1708977569)
nit: It might be worth renaming this to `GetFee` or `EvaluateFee`. The helper function and fuzzing comments refer to Evaluate to explain the calculations.
While it's clear they aren't the same, differentiating explicitly by stating whats being evaluated might be better.
(https://github.com/bitcoin/bitcoin/pull/30535#discussion_r1708977569)
nit: It might be worth renaming this to `GetFee` or `EvaluateFee`. The helper function and fuzzing comments refer to Evaluate to explain the calculations.
While it's clear they aren't the same, differentiating explicitly by stating whats being evaluated might be better.
💬 hebasto commented on pull request "build: Introduce CMake-based build system":
(https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1709396728)
This PR is intended to focus solely on migration. It helps to review it a lot.
> Should probably be done in a different PR, though...
I agree.
(https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1709396728)
This PR is intended to focus solely on migration. It helps to review it a lot.
> Should probably be done in a different PR, though...
I agree.
✅ fjahr closed a pull request: "Assumeutxo: Sanitize block height in metadata"
(https://github.com/bitcoin/bitcoin/pull/30516)
(https://github.com/bitcoin/bitcoin/pull/30516)
💬 fjahr commented on pull request "Assumeutxo: Sanitize block height in metadata":
(https://github.com/bitcoin/bitcoin/pull/30516#issuecomment-2275715938)
It doesn't seem like we can come to an agreement quickly here, so I am closing this in favor of #30598 so we can move forward with the mainnet params. I would appreciate your reviews there, thank you!
(https://github.com/bitcoin/bitcoin/pull/30516#issuecomment-2275715938)
It doesn't seem like we can come to an agreement quickly here, so I am closing this in favor of #30598 so we can move forward with the mainnet params. I would appreciate your reviews there, thank you!
💬 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. :)