💬 hodlinator commented on pull request "node: reduce unsafe uint256S usage":
(https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1709346477)
The inclination to use `Assert` for any fatal error is understandable, but as I said in my book they are for catching logic errors - not for validating user environment. My non-Bitcoin experience tells me that policies around what should be asserts require upkeep, so I took the time to provide an alternate version.
Was reading https://bitcoincore.academy/bin/onboarding-to-bitcoin-core.html today and came across
> We take extra care during the encryption phase to either complete atomically or
...
(https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1709346477)
The inclination to use `Assert` for any fatal error is understandable, but as I said in my book they are for catching logic errors - not for validating user environment. My non-Bitcoin experience tells me that policies around what should be asserts require upkeep, so I took the time to provide an alternate version.
Was reading https://bitcoincore.academy/bin/onboarding-to-bitcoin-core.html today and came across
> We take extra care during the encryption phase to either complete atomically or
...
💬 paplorinc commented on pull request "node: reduce unsafe uint256S usage":
(https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1709362894)
Given that:
* this is only used in a single place in prod (which accept `std::string_view` as well and has an `std::string` input)
* we're always copying the result before returning
* the overwhelming majority of inputs will probably have the correct size already, i.e. don't need change.
* the performance of the method seems to be important
Could we either return an `std::optional<std::string_view>` instead or take an `std::string& input` to be able to return it?
i.e.
```suggestion
s
...
(https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1709362894)
Given that:
* this is only used in a single place in prod (which accept `std::string_view` as well and has an `std::string` input)
* we're always copying the result before returning
* the overwhelming majority of inputs will probably have the correct size already, i.e. don't need change.
* the performance of the method seems to be important
Could we either return an `std::optional<std::string_view>` instead or take an `std::string& input` to be able to return it?
i.e.
```suggestion
s
...
📝 fanquake opened a pull request: "guix: bump time-machine to 7bf1d7aeaffba15c4f680f93ae88fbef25427252"
(https://github.com/bitcoin/bitcoin/pull/30609)
Includes:
https://git.savannah.gnu.org/cgit/guix.git/commit/?id=d428237642e1e4ac8fda4597205ffec89926c0ec.
which removes the need to build Python2, and OpenSSL `1.x` (which has historically caused issues) when building for Windows (Python2 (with a dependency on OpenSSL `1.x`) used to be a dependency of NSIS).
Linux Kernel Headers `6.1.100` -> `6.1.102`
```bash
d079858fb1bc526217ee06f312d97a56c34986440e5f9e108af66eaecacea073 guix-build-eca20bead2da/output/aarch64-linux-gnu/SHA256SUMS.p
...
(https://github.com/bitcoin/bitcoin/pull/30609)
Includes:
https://git.savannah.gnu.org/cgit/guix.git/commit/?id=d428237642e1e4ac8fda4597205ffec89926c0ec.
which removes the need to build Python2, and OpenSSL `1.x` (which has historically caused issues) when building for Windows (Python2 (with a dependency on OpenSSL `1.x`) used to be a dependency of NSIS).
Linux Kernel Headers `6.1.100` -> `6.1.102`
```bash
d079858fb1bc526217ee06f312d97a56c34986440e5f9e108af66eaecacea073 guix-build-eca20bead2da/output/aarch64-linux-gnu/SHA256SUMS.p
...
💬 fanquake commented on pull request "guix: fix suggested fake date for openssl-1.1.1l":
(https://github.com/bitcoin/bitcoin/pull/29999#issuecomment-2275687665)
See https://github.com/bitcoin/bitcoin/pull/30609.
(https://github.com/bitcoin/bitcoin/pull/29999#issuecomment-2275687665)
See https://github.com/bitcoin/bitcoin/pull/30609.
💬 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