🚀 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
🤔 glozow reviewed a pull request: "mining: drop unused -nFees and sigops from CBlockTemplate"
(https://github.com/bitcoin/bitcoin/pull/31897#pullrequestreview-2679788820)
looks correct, untested ACK 6d7648795aa053f535510d11379fdd9d0399004c
(https://github.com/bitcoin/bitcoin/pull/31897#pullrequestreview-2679788820)
looks correct, untested ACK 6d7648795aa053f535510d11379fdd9d0399004c
💬 glozow commented on pull request "mining: drop unused -nFees and sigops from CBlockTemplate":
(https://github.com/bitcoin/bitcoin/pull/31897#discussion_r1992245751)
When I change this to `int index_in_template = 0;` or make changes to the values pushed for "fee" and "depends" none of the functional tests fail. Could be worth adding coverage?
(https://github.com/bitcoin/bitcoin/pull/31897#discussion_r1992245751)
When I change this to `int index_in_template = 0;` or make changes to the values pushed for "fee" and "depends" none of the functional tests fail. Could be worth adding coverage?
💬 darosior commented on pull request "fuzz: Expand script verification flag testing to segwit v0 and tapscript":
(https://github.com/bitcoin/bitcoin/pull/31460#discussion_r1992326301)
Same here, you could just variate based on the value itself?
(https://github.com/bitcoin/bitcoin/pull/31460#discussion_r1992326301)
Same here, you could just variate based on the value itself?