👍 hebasto approved a pull request: "[29.x] bump to v29.0rc1"
(https://github.com/bitcoin/bitcoin/pull/32046#pullrequestreview-2679617137)
ACK 47e2fa86dc5433852fd9e5050a23de2accfdca8d.
(https://github.com/bitcoin/bitcoin/pull/32046#pullrequestreview-2679617137)
ACK 47e2fa86dc5433852fd9e5050a23de2accfdca8d.
🚀 ryanofsky merged a pull request: "Add waitNext() to BlockTemplate interface"
(https://github.com/bitcoin/bitcoin/pull/31283)
(https://github.com/bitcoin/bitcoin/pull/31283)
👍 ryanofsky approved a pull request: "[IBD] flush UXTOs in bigger batches"
(https://github.com/bitcoin/bitcoin/pull/31645#pullrequestreview-2679668648)
Code review ACK 868413340f8d6058d74186b65ac3498d6b7f254a
> If that spike exceeds the memory the process can allocate it causes a crash, at a particularly bad time (may require a replay to fix, which may be slower than just reprocessing the blocks).
It is difficult for me to have a sense of how safe this change is but I'd hope we are not currently pushing systems so close to the edge that using an extra 48mb will cause them to start crashing. This does seem like a nice performance improvmen
...
(https://github.com/bitcoin/bitcoin/pull/31645#pullrequestreview-2679668648)
Code review ACK 868413340f8d6058d74186b65ac3498d6b7f254a
> If that spike exceeds the memory the process can allocate it causes a crash, at a particularly bad time (may require a replay to fix, which may be slower than just reprocessing the blocks).
It is difficult for me to have a sense of how safe this change is but I'd hope we are not currently pushing systems so close to the edge that using an extra 48mb will cause them to start crashing. This does seem like a nice performance improvmen
...
🚀 hebasto merged a pull request: "wallet: Replace "non-0" with "non-zero" in translatable error message"
(https://github.com/bitcoin/bitcoin/pull/31987)
(https://github.com/bitcoin/bitcoin/pull/31987)
💬 mabu44 commented on pull request "test: Check datadir cleanup after assumeutxo was successful":
(https://github.com/bitcoin/bitcoin/pull/32033#issuecomment-2718935452)
Tested the PR with the following steps:
#### On master:
```bash
python3 feature_assumeutxo.py
...
TestFramework (INFO): Tests successful
```
Modified validation.cpp (line 6237, function MaybeCompleteSnapshotValidation):
```diff
- return SnapshotCompletionResult::SUCCESS;
+ return SnapshotCompletionResult::SKIPPED;
```
This should avoid the clean-up of the snapshot at startup. But the test is not affected:
```bash
python3 feature_assumeutxo.py
...
TestFramework (INFO): Tests succe
...
(https://github.com/bitcoin/bitcoin/pull/32033#issuecomment-2718935452)
Tested the PR with the following steps:
#### On master:
```bash
python3 feature_assumeutxo.py
...
TestFramework (INFO): Tests successful
```
Modified validation.cpp (line 6237, function MaybeCompleteSnapshotValidation):
```diff
- return SnapshotCompletionResult::SUCCESS;
+ return SnapshotCompletionResult::SKIPPED;
```
This should avoid the clean-up of the snapshot at startup. But the test is not affected:
```bash
python3 feature_assumeutxo.py
...
TestFramework (INFO): Tests succe
...
💬 jamesob commented on pull request "BIP-119 (OP_CHECKTEMPLATEVERIFY) (no activation)":
(https://github.com/bitcoin/bitcoin/pull/31989#issuecomment-2718981226)
I've added a link to the active [Delving Bitcoin discussion](https://delvingbitcoin.org/t/ctv-csfs-can-we-reach-consensus-on-a-first-step-towards-covenants/1509/23) most pertaining to CTV, and will keep the PR description above updated with any other relevant discussions.
(https://github.com/bitcoin/bitcoin/pull/31989#issuecomment-2718981226)
I've added a link to the active [Delving Bitcoin discussion](https://delvingbitcoin.org/t/ctv-csfs-can-we-reach-consensus-on-a-first-step-towards-covenants/1509/23) most pertaining to CTV, and will keep the PR description above updated with any other relevant discussions.
💬 jonatack commented on pull request "refactor: CLI cleanups":
(https://github.com/bitcoin/bitcoin/pull/31887#discussion_r1992220870)
> hoist up the braces to conform with the dev-notes? Also...inconsistent spacing
> it would be good to use recommended style (can just run `clang-format`)
Good idea, done.
(https://github.com/bitcoin/bitcoin/pull/31887#discussion_r1992220870)
> hoist up the braces to conform with the dev-notes? Also...inconsistent spacing
> it would be good to use recommended style (can just run `clang-format`)
Good idea, done.
💬 jonatack commented on pull request "refactor: CLI cleanups":
(https://github.com/bitcoin/bitcoin/pull/31887#discussion_r1992223671)
Oh ok, updated the commit message per s/upper bound/bound/ as you suggested.
(https://github.com/bitcoin/bitcoin/pull/31887#discussion_r1992223671)
Oh ok, updated the commit message per s/upper bound/bound/ as you suggested.
💬 jonatack commented on pull request "refactor: CLI cleanups":
(https://github.com/bitcoin/bitcoin/pull/31887#discussion_r1992234119)
See https://stackoverflow.com/questions/3574040/c-can-a-struct-inherit-from-a-class in addition to my response above.
(https://github.com/bitcoin/bitcoin/pull/31887#discussion_r1992234119)
See https://stackoverflow.com/questions/3574040/c-can-a-struct-inherit-from-a-class in addition to my response above.
💬 jonatack commented on pull request "refactor: CLI cleanups":
(https://github.com/bitcoin/bitcoin/pull/31887#discussion_r1992240148)
Thank you for clarifying the change. Done.
(https://github.com/bitcoin/bitcoin/pull/31887#discussion_r1992240148)
Thank you for clarifying the change. Done.
⚠️ yancyribbens opened an issue: "BnB untested/unused condition in UTXO exclusion optimization"
(https://github.com/bitcoin/bitcoin/issues/32047)
If the following condition is remove, no failure occurs:
`utxo.fee != utxo_pool.at(utxo_pool_index - 1).fee)`
https://github.com/bitcoin/bitcoin/blob/eb9730ab65887279309af338ad0201924f945ac5/src/wallet/coinselection.cpp#L177
Furthermore, I believe that this condition isn't actually needed and can be removed, which would slightly boost performance. The section is checking if a UTXO can be omitted from evaluation if it has the same effective_value as the previous UTXO and the previous was alrea
...
(https://github.com/bitcoin/bitcoin/issues/32047)
If the following condition is remove, no failure occurs:
`utxo.fee != utxo_pool.at(utxo_pool_index - 1).fee)`
https://github.com/bitcoin/bitcoin/blob/eb9730ab65887279309af338ad0201924f945ac5/src/wallet/coinselection.cpp#L177
Furthermore, I believe that this condition isn't actually needed and can be removed, which would slightly boost performance. The section is checking if a UTXO can be omitted from evaluation if it has the same effective_value as the previous UTXO and the previous was alrea
...
💬 jonatack commented on pull request "refactor: CLI cleanups":
(https://github.com/bitcoin/bitcoin/pull/31887#issuecomment-2719061577)
Thank you for reviewing, @pablomartin4btc. I prefer to keep this pull scoped only to this file and not increase the scope, as these changes are local to it and it is a file I know reasonably well (the RequestHandler representation hasn't changed over that time period).
(https://github.com/bitcoin/bitcoin/pull/31887#issuecomment-2719061577)
Thank you for reviewing, @pablomartin4btc. I prefer to keep this pull scoped only to this file and not increase the scope, as these changes are local to it and it is a file I know reasonably well (the RequestHandler representation hasn't changed over that time period).
💬 hodlinator commented on pull request "refactor: CLI cleanups":
(https://github.com/bitcoin/bitcoin/pull/31887#discussion_r1992250931)
My objection does not come from a knowledge gap in what C++ can and cannot do. It is about sticking to certain conventions.
As I was checking to see if the Core Guidelines had influenced me in the matter I only found weak counter evidence:
https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#c122-use-abstract-classes-as-interfaces-when-complete-separation-of-interface-and-implementation-is-needed
In the example code `class D1` inherits from `struct Device`, full of virtual methods.
(https://github.com/bitcoin/bitcoin/pull/31887#discussion_r1992250931)
My objection does not come from a knowledge gap in what C++ can and cannot do. It is about sticking to certain conventions.
As I was checking to see if the Core Guidelines had influenced me in the matter I only found weak counter evidence:
https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#c122-use-abstract-classes-as-interfaces-when-complete-separation-of-interface-and-implementation-is-needed
In the example code `class D1` inherits from `struct Device`, full of virtual methods.
💬 davidgumberg commented on pull request "depends: remove `NO_HARDEN` option":
(https://github.com/bitcoin/bitcoin/pull/32038#issuecomment-2719073609)
crACK 5dfef6b9b379f51e
Sanity tested that the following depends build works:
```bash
make -C depends/ HOST=x86_64-w64-mingw32
cmake -B build --toolchain depends/x86_64-w64-mingw32/toolchain.cmake
cmake --build build
```
---
> for a project like this it's unreasonable to disable hardening features
Would it make sense to also drop `ENABLE_HARDENING`? Currently set `OFF` in the `clang-tidy` ci job, but I [flipped it `ON`](https://github.com/davidgumberg/bitcoin/commit/ce79bea14ae
...
(https://github.com/bitcoin/bitcoin/pull/32038#issuecomment-2719073609)
crACK 5dfef6b9b379f51e
Sanity tested that the following depends build works:
```bash
make -C depends/ HOST=x86_64-w64-mingw32
cmake -B build --toolchain depends/x86_64-w64-mingw32/toolchain.cmake
cmake --build build
```
---
> for a project like this it's unreasonable to disable hardening features
Would it make sense to also drop `ENABLE_HARDENING`? Currently set `OFF` in the `clang-tidy` ci job, but I [flipped it `ON`](https://github.com/davidgumberg/bitcoin/commit/ce79bea14ae
...
💬 jonatack commented on pull request "refactor: CLI cleanups":
(https://github.com/bitcoin/bitcoin/pull/31887#issuecomment-2719074019)
Thanks for the reviews, everyone. Ran clang-format on the third commit and adjusted a few commit messages per the feedback. Should be trivial to re-ack (thanks!)
(https://github.com/bitcoin/bitcoin/pull/31887#issuecomment-2719074019)
Thanks for the reviews, everyone. Ran clang-format on the third commit and adjusted a few commit messages per the feedback. Should be trivial to re-ack (thanks!)
💬 yancyribbens commented on issue "BnB untested/unused condition in UTXO exclusion optimization":
(https://github.com/bitcoin/bitcoin/issues/32047#issuecomment-2719074335)
cc @murchandamus
(https://github.com/bitcoin/bitcoin/issues/32047#issuecomment-2719074335)
cc @murchandamus
💬 jonatack commented on pull request "refactor: CLI cleanups":
(https://github.com/bitcoin/bitcoin/pull/31887#discussion_r1992261225)
I believe I already addressed the conventions comment with https://github.com/bitcoin/bitcoin/pull/31887#discussion_r1966566982.
(https://github.com/bitcoin/bitcoin/pull/31887#discussion_r1992261225)
I believe I already addressed the conventions comment with https://github.com/bitcoin/bitcoin/pull/31887#discussion_r1966566982.
📝 VolodymyrBg opened a pull request: "docs: Enhance TODO section in README.md with detailed explanations"
(https://github.com/bitcoin/bitcoin/pull/32048)
This pull request improves the TODO section in the Minisketch README.md by:
1. Adding detailed explanations for each planned improvement:
- Explicit formulas for higher-degree polynomials
- Subquadratic multiplication and modulus algorithms
- Half-GCD algorithm implementation
- Incremental decoding interface
- Platform-specific optimizations beyond x86
- 32-bit host optimizations
- IBLT/Hybrid approaches
2. Introducing implementation priorities to guide future development efforts:
- Pr
...
(https://github.com/bitcoin/bitcoin/pull/32048)
This pull request improves the TODO section in the Minisketch README.md by:
1. Adding detailed explanations for each planned improvement:
- Explicit formulas for higher-degree polynomials
- Subquadratic multiplication and modulus algorithms
- Half-GCD algorithm implementation
- Incremental decoding interface
- Platform-specific optimizations beyond x86
- 32-bit host optimizations
- IBLT/Hybrid approaches
2. Introducing implementation priorities to guide future development efforts:
- Pr
...
💬 yancyribbens commented on issue "BnB untested/unused condition in UTXO exclusion optimization":
(https://github.com/bitcoin/bitcoin/issues/32047#issuecomment-2719110760)
I also just wanted to add that, even if there was the extremely unlikely event where two utxos had the same effective_value and different fee values, the optimization should only compare effective_values as I understand it.
(https://github.com/bitcoin/bitcoin/issues/32047#issuecomment-2719110760)
I also just wanted to add that, even if there was the extremely unlikely event where two utxos had the same effective_value and different fee values, the optimization should only compare effective_values as I understand it.
🤔 pablomartin4btc reviewed a pull request: "refactor: CLI cleanups"
(https://github.com/bitcoin/bitcoin/pull/31887#pullrequestreview-2679864084)
re-ACK d423fd9ec83ae323ac29bdc1b677ed8260bd59c4
(https://github.com/bitcoin/bitcoin/pull/31887#pullrequestreview-2679864084)
re-ACK d423fd9ec83ae323ac29bdc1b677ed8260bd59c4