π¬ l0rinc commented on pull request "common: Make arith_uint256 trivially copyable":
(https://github.com/bitcoin/bitcoin/pull/33332#issuecomment-3281739187)
ACK 653a9849d5f98ba80e334ddc0ae9a5e367459f59
I'm fine with the change as it is currently, would prefer more implicit behavior instead (the original PR), but this is also better than before, thanks for doing it.
(https://github.com/bitcoin/bitcoin/pull/33332#issuecomment-3281739187)
ACK 653a9849d5f98ba80e334ddc0ae9a5e367459f59
I'm fine with the change as it is currently, would prefer more implicit behavior instead (the original PR), but this is also better than before, thanks for doing it.
π l0rinc's pull request is ready for review: "coins: warn on oversized `-dbcache`"
(https://github.com/bitcoin/bitcoin/pull/33333)
(https://github.com/bitcoin/bitcoin/pull/33333)
π€ enirox001 reviewed a pull request: "test/refactor: use test deque to avoid quadratic iteration"
(https://github.com/bitcoin/bitcoin/pull/33313#pullrequestreview-3212757868)
ACK https://github.com/bitcoin/bitcoin/commit/8c1f10233dc9a843147772c40973031f5bfdbb7c. This is a solid improvement. While the benefits may not be immediately obvious, itβs a valuable addition. I ran the functional tests and everything passed successfully.
(https://github.com/bitcoin/bitcoin/pull/33313#pullrequestreview-3212757868)
ACK https://github.com/bitcoin/bitcoin/commit/8c1f10233dc9a843147772c40973031f5bfdbb7c. This is a solid improvement. While the benefits may not be immediately obvious, itβs a valuable addition. I ran the functional tests and everything passed successfully.
π¬ sipa commented on pull request "cluster mempool: control/optimize TxGraph memory usage":
(https://github.com/bitcoin/bitcoin/pull/33157#issuecomment-3281939547)
Rebased after #33354 merge.
(https://github.com/bitcoin/bitcoin/pull/33157#issuecomment-3281939547)
Rebased after #33354 merge.
π¬ willcl-ark commented on pull request "coins: warn on oversized `-dbcache`":
(https://github.com/bitcoin/bitcoin/pull/33333#issuecomment-3282061941)
Concept ACK
I think warning when someone is likely going to clobber their system performance is useful.
This change introduces a warning on lower limit of free RAM remaining; would it make sense to also include an upper bound to suppress/skip the warning? The cap is set to warn when < 1.5GB is left free in the case RAM is below 2GB, but will then warn when I have 16GB of ram free:
```
src/core/bitcoin on ξ pr-33333 [$] via β³ v3.31.6 via π v3.13.3 via βοΈ impure (nix-shell-env) took 3s
...
(https://github.com/bitcoin/bitcoin/pull/33333#issuecomment-3282061941)
Concept ACK
I think warning when someone is likely going to clobber their system performance is useful.
This change introduces a warning on lower limit of free RAM remaining; would it make sense to also include an upper bound to suppress/skip the warning? The cap is set to warn when < 1.5GB is left free in the case RAM is below 2GB, but will then warn when I have 16GB of ram free:
```
src/core/bitcoin on ξ pr-33333 [$] via β³ v3.31.6 via π v3.13.3 via βοΈ impure (nix-shell-env) took 3s
...
π¬ davidgumberg commented on pull request "ci: always use tag for LLVM checkout":
(https://github.com/bitcoin/bitcoin/pull/33364#issuecomment-3282063472)
ACK https://github.com/bitcoin/bitcoin/commit/b736052e39f1f466f63f261ace3dd2deba171e8a
(https://github.com/bitcoin/bitcoin/pull/33364#issuecomment-3282063472)
ACK https://github.com/bitcoin/bitcoin/commit/b736052e39f1f466f63f261ace3dd2deba171e8a
π darosior approved a pull request: "Bump SCRIPT_VERIFY flags to 64 bit"
(https://github.com/bitcoin/bitcoin/pull/32998#pullrequestreview-3212374162)
ACK 652424ad162b63d73ecb6bd65bd26946e90c617f
I think it would also make sense, in a follow-up, to move the rest of the Script verification flags stuff to the new `verify_flags.h`.
(https://github.com/bitcoin/bitcoin/pull/32998#pullrequestreview-3212374162)
ACK 652424ad162b63d73ecb6bd65bd26946e90c617f
I think it would also make sense, in a follow-up, to move the rest of the Script verification flags stuff to the new `verify_flags.h`.
π¬ darosior commented on pull request "Bump SCRIPT_VERIFY flags to 64 bit":
(https://github.com/bitcoin/bitcoin/pull/32998#discussion_r2341455180)
I was curious why this constructor from `int` with a runtime check was necessary. It's because of all the `if ((flags & SCRIPT_VERIFY_X) != 0)` in the interpreter. This permits to implicitly construct a `script_verify_flags` from the `0` there instead of either having an implicit conversion *to* integer (the very thing this aims to avoid), or touching every single of those checks to make the conversion from the `0` integer explicit.
(https://github.com/bitcoin/bitcoin/pull/32998#discussion_r2341455180)
I was curious why this constructor from `int` with a runtime check was necessary. It's because of all the `if ((flags & SCRIPT_VERIFY_X) != 0)` in the interpreter. This permits to implicitly construct a `script_verify_flags` from the `0` there instead of either having an implicit conversion *to* integer (the very thing this aims to avoid), or touching every single of those checks to make the conversion from the `0` integer explicit.
π¬ darosior commented on pull request "Bump SCRIPT_VERIFY flags to 64 bit":
(https://github.com/bitcoin/bitcoin/pull/32998#discussion_r2341832137)
How is the rule of 5 relevant for this class? It does not need a user-defined destructor.
From cppreference's [Rule of 5 section](https://en.cppreference.com/w/cpp/language/rule_of_three.html#Rule_of_five):
> Because the presence of a user-defined (include = default or = delete declared) destructor, copy-constructor, or copy-assignment operator prevents implicit definition of the [move constructor](https://en.cppreference.com/w/cpp/language/move_constructor.html) and the [move assignment ope
...
(https://github.com/bitcoin/bitcoin/pull/32998#discussion_r2341832137)
How is the rule of 5 relevant for this class? It does not need a user-defined destructor.
From cppreference's [Rule of 5 section](https://en.cppreference.com/w/cpp/language/rule_of_three.html#Rule_of_five):
> Because the presence of a user-defined (include = default or = delete declared) destructor, copy-constructor, or copy-assignment operator prevents implicit definition of the [move constructor](https://en.cppreference.com/w/cpp/language/move_constructor.html) and the [move assignment ope
...
π¬ instagibbs commented on pull request "Bump SCRIPT_VERIFY flags to 64 bit":
(https://github.com/bitcoin/bitcoin/pull/32998#issuecomment-3282095349)
reACK https://github.com/bitcoin/bitcoin/pull/32998/commits/652424ad162b63d73ecb6bd65bd26946e90c617f
(https://github.com/bitcoin/bitcoin/pull/32998#issuecomment-3282095349)
reACK https://github.com/bitcoin/bitcoin/pull/32998/commits/652424ad162b63d73ecb6bd65bd26946e90c617f
π¬ l0rinc commented on pull request "coins: warn on oversized `-dbcache`":
(https://github.com/bitcoin/bitcoin/pull/33333#issuecomment-3282103704)
> would it make sense to also include an upper bound to suppress/skip the warning
I'm afraid it would be interpreted as a recommendation in that case - all we can claim is that this is likely too high, we can't actually know the optimal size
> feels a shame to introduce new false positives if we can avoid it?
You mean that the uxto size is less than 48 GiB therefore the warning isn't relevant?
(https://github.com/bitcoin/bitcoin/pull/33333#issuecomment-3282103704)
> would it make sense to also include an upper bound to suppress/skip the warning
I'm afraid it would be interpreted as a recommendation in that case - all we can claim is that this is likely too high, we can't actually know the optimal size
> feels a shame to introduce new false positives if we can avoid it?
You mean that the uxto size is less than 48 GiB therefore the warning isn't relevant?
π¬ theuni commented on pull request "guix: build for Linux HOSTS with `-static-libgcc`":
(https://github.com/bitcoin/bitcoin/pull/33181#discussion_r2342101039)
Hmm, is it necessary for `CMAKE_EXE_LINKER_FLAGS` to also contain `HOST_LDFLAGS`? I would have expected CMake to use both for exe's.
(https://github.com/bitcoin/bitcoin/pull/33181#discussion_r2342101039)
Hmm, is it necessary for `CMAKE_EXE_LINKER_FLAGS` to also contain `HOST_LDFLAGS`? I would have expected CMake to use both for exe's.
π¬ katesalazar commented on pull request "policy: lower the default blockmintxfee, incrementalrelayfee, minrelaytxfee":
(https://github.com/bitcoin/bitcoin/pull/33106#issuecomment-3282309789)
ty
(https://github.com/bitcoin/bitcoin/pull/33106#issuecomment-3282309789)
ty
π¬ fanquake commented on pull request "guix: build for Linux HOSTS with `-static-libgcc`":
(https://github.com/bitcoin/bitcoin/pull/33181#discussion_r2342105925)
I couldn't find a way to pass these through, without it, without the build breaking in one way or another.
(https://github.com/bitcoin/bitcoin/pull/33181#discussion_r2342105925)
I couldn't find a way to pass these through, without it, without the build breaking in one way or another.
β οΈ janb84 opened an issue: "30.0 RC Testing Guide Feedback"
(https://github.com/bitcoin/bitcoin/issues/33369)
This issue is to discuss the [30.0 Release Candidate Testing Guide](https://github.com/bitcoin-core/bitcoin-devwiki/wiki/30.0-Release-Candidate-Testing-Guide). If you have any feedback on the document, please leave a comment here.
Note: This is for feedback on the document, not on Bitcoin Core or on the 30.0 changes. Create a new issue or give feedback in the [v30.0 Testing](https://github.com/bitcoin/bitcoin/issues/33368) issue
Thank you for taking a look at the guide and leaving your feedbac
...
(https://github.com/bitcoin/bitcoin/issues/33369)
This issue is to discuss the [30.0 Release Candidate Testing Guide](https://github.com/bitcoin-core/bitcoin-devwiki/wiki/30.0-Release-Candidate-Testing-Guide). If you have any feedback on the document, please leave a comment here.
Note: This is for feedback on the document, not on Bitcoin Core or on the 30.0 changes. Create a new issue or give feedback in the [v30.0 Testing](https://github.com/bitcoin/bitcoin/issues/33368) issue
Thank you for taking a look at the guide and leaving your feedbac
...
π¬ achow101 commented on pull request "test: Remove polling loop from test_runner (take 2)":
(https://github.com/bitcoin/bitcoin/pull/33141#issuecomment-3282443734)
ACK fa4885ef2fde2b9fd9eb7367564a2946a45f20ac
(https://github.com/bitcoin/bitcoin/pull/33141#issuecomment-3282443734)
ACK fa4885ef2fde2b9fd9eb7367564a2946a45f20ac
π€ furszy reviewed a pull request: "wallet: Identify transactions spending 0-value outputs, and add tests for anchor outputs in a wallet"
(https://github.com/bitcoin/bitcoin/pull/33268#pullrequestreview-3213451317)
ACK 113a4228229baedda2a730e097f2d59ad58a4b0d
(https://github.com/bitcoin/bitcoin/pull/33268#pullrequestreview-3213451317)
ACK 113a4228229baedda2a730e097f2d59ad58a4b0d
π achow101 merged a pull request: "test: Remove polling loop from test_runner (take 2)"
(https://github.com/bitcoin/bitcoin/pull/33141)
(https://github.com/bitcoin/bitcoin/pull/33141)
π¬ achow101 commented on pull request "kernel: make blockTip index const":
(https://github.com/bitcoin/bitcoin/pull/33321#issuecomment-3282473712)
ACK 75d9b72475708ee0da13fb23ef65dcced805b6af
(https://github.com/bitcoin/bitcoin/pull/33321#issuecomment-3282473712)
ACK 75d9b72475708ee0da13fb23ef65dcced805b6af
π¬ l0rinc commented on pull request "test/refactor: use test deque to avoid quadratic iteration":
(https://github.com/bitcoin/bitcoin/pull/33313#issuecomment-3282473980)
Thanks for the reviews so far, I have rebased it after https://github.com/bitcoin/bitcoin/pull/33141, should be trivial to re-review
(https://github.com/bitcoin/bitcoin/pull/33313#issuecomment-3282473980)
Thanks for the reviews so far, I have rebased it after https://github.com/bitcoin/bitcoin/pull/33141, should be trivial to re-review