💬 paplorinc commented on pull request "build: Introduce CMake-based build system":
(https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1709187478)
nit: slightly unrelated, but in other cases similar code blocks are annotated as either "\```bash" or "```sh" which are treated a bit different in e.g. CLion:
<img src="https://github.com/user-attachments/assets/13f30a0a-1f92-4a39-87ce-8ca11a05a44f">
Also note that "\``` bash" is also a formatted differently than "```bash".
Should probably be done in a different PR, though...
(https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1709187478)
nit: slightly unrelated, but in other cases similar code blocks are annotated as either "\```bash" or "```sh" which are treated a bit different in e.g. CLion:
<img src="https://github.com/user-attachments/assets/13f30a0a-1f92-4a39-87ce-8ca11a05a44f">
Also note that "\``` bash" is also a formatted differently than "```bash".
Should probably be done in a different PR, though...
💬 paplorinc commented on pull request "build: Introduce CMake-based build system":
(https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1709167244)
nit: "Python 3" is a bit confusing here since Python 2 was sunset in 2020, and we don't accept all 3.x versions.
Would it make sense to simplify these "Python 3" references to just "Python"?
(https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1709167244)
nit: "Python 3" is a bit confusing here since Python 2 was sunset in 2020, and we don't accept all 3.x versions.
Would it make sense to simplify these "Python 3" references to just "Python"?
💬 paplorinc commented on pull request "build: Introduce CMake-based build system":
(https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1709145204)
👍 GUI is working with cmake, but I had to execute a `brew link qt5 --force` as well after `brew install qt@5` (applies to Autotools as well)
(https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1709145204)
👍 GUI is working with cmake, but I had to execute a `brew link qt5 --force` as well after `brew install qt@5` (applies to Autotools as well)
💬 paplorinc commented on pull request "build: Introduce CMake-based build system":
(https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1709278334)
Is there any way I could help here?
(https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1709278334)
Is there any way I could help here?
💬 paplorinc commented on pull request "build: Introduce CMake-based build system":
(https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1709270807)
Wanted to suggest`"([A-Za-z0-9]{2})"` (or `\w{2}`), but it seems the regex parser was written almost 40 years ago https://github.com/Kitware/CMake/blob/master/Source/kwsys/RegularExpression.cxx#L628
(https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1709270807)
Wanted to suggest`"([A-Za-z0-9]{2})"` (or `\w{2}`), but it seems the regex parser was written almost 40 years ago https://github.com/Kitware/CMake/blob/master/Source/kwsys/RegularExpression.cxx#L628
💬 Sjors commented on pull request "guix: fix suggested fake date for openssl-1.1.1l":
(https://github.com/bitcoin/bitcoin/pull/29999#issuecomment-2275621895)
Would be quite nice to get rid of openssl-1.1.1 entirely. If the upstream is merged and adopted here, I might nuke my Guix setup to try it.
(https://github.com/bitcoin/bitcoin/pull/29999#issuecomment-2275621895)
Would be quite nice to get rid of openssl-1.1.1 entirely. If the upstream is merged and adopted here, I might nuke my Guix setup to try it.
💬 maflcko commented on pull request "Assumeutxo: Sanitize block height in metadata":
(https://github.com/bitcoin/bitcoin/pull/30516#issuecomment-2275639826)
> This PR fixes the undefined behavior that the sanitizer picked up in #30514.
I don't think this is UB. Personally I couldn't care less about a implicit fuzz integer issue, and I don't think any production user cares either, because fuzz tests aren't run in production.
I don't see the value in only silencing the fuzz target, but leaving the other reported bugs unfixed. If silencing the fuzz target was a goal, it could be done with a trivial temporary one-line patch to the suppressions fil
...
(https://github.com/bitcoin/bitcoin/pull/30516#issuecomment-2275639826)
> This PR fixes the undefined behavior that the sanitizer picked up in #30514.
I don't think this is UB. Personally I couldn't care less about a implicit fuzz integer issue, and I don't think any production user cares either, because fuzz tests aren't run in production.
I don't see the value in only silencing the fuzz target, but leaving the other reported bugs unfixed. If silencing the fuzz target was a goal, it could be done with a trivial temporary one-line patch to the suppressions fil
...
💬 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!