π 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
π hodlinator approved a pull request: "common: Make arith_uint256 trivially copyable"
(https://github.com/bitcoin/bitcoin/pull/33332#pullrequestreview-3213497518)
re-ACK 653a9849d5f98ba80e334ddc0ae9a5e367459f59
New push only adds `static_assert` with motivation.
(https://github.com/bitcoin/bitcoin/pull/33332#pullrequestreview-3213497518)
re-ACK 653a9849d5f98ba80e334ddc0ae9a5e367459f59
New push only adds `static_assert` with motivation.
π BrandonOdiwuor opened a pull request: "ci: use Mold linker for asan-lsan-ubsan-integer-no-depends-usdt workflow"
(https://github.com/bitcoin/bitcoin/pull/33370)
Follow up to https://github.com/bitcoin/bitcoin/pull/32888#pullrequestreview-2993523631 and https://github.com/bitcoin/bitcoin/pull/32888#issuecomment-3044773485
>>Can we use `mold` as a linker in other Linux based system workflows ? dependencies [we have](https://github.com/bitcoin/bitcoin/blob/master/doc/dependencies.md#compiler) seem to satisfy the deps here https://github.com/rui314/mold?tab=readme-ov-file#how-to-build
>
> Sure, happy to review a follow-up. Only place to avoid it would
...
(https://github.com/bitcoin/bitcoin/pull/33370)
Follow up to https://github.com/bitcoin/bitcoin/pull/32888#pullrequestreview-2993523631 and https://github.com/bitcoin/bitcoin/pull/32888#issuecomment-3044773485
>>Can we use `mold` as a linker in other Linux based system workflows ? dependencies [we have](https://github.com/bitcoin/bitcoin/blob/master/doc/dependencies.md#compiler) seem to satisfy the deps here https://github.com/rui314/mold?tab=readme-ov-file#how-to-build
>
> Sure, happy to review a follow-up. Only place to avoid it would
...