💬 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?
🤔 darosior reviewed a pull request: "fuzz: Expand script verification flag testing to segwit v0 and tapscript"
(https://github.com/bitcoin/bitcoin/pull/31460#pullrequestreview-2679919459)
Concept ACK.
(https://github.com/bitcoin/bitcoin/pull/31460#pullrequestreview-2679919459)
Concept ACK.
💬 darosior commented on pull request "fuzz: Expand script verification flag testing to segwit v0 and tapscript":
(https://github.com/bitcoin/bitcoin/pull/31460#discussion_r1992325287)
What does hashing adds here? If you need more than one bit why not take it out of `sig` directly (and have a default value if it's too small)?
(https://github.com/bitcoin/bitcoin/pull/31460#discussion_r1992325287)
What does hashing adds here? If you need more than one bit why not take it out of `sig` directly (and have a default value if it's too small)?
💬 darosior commented on pull request "fuzz: Expand script verification flag testing to segwit v0 and tapscript":
(https://github.com/bitcoin/bitcoin/pull/31460#discussion_r1992323288)
This seems unnecessary, none of that is used.
(https://github.com/bitcoin/bitcoin/pull/31460#discussion_r1992323288)
This seems unnecessary, none of that is used.
💬 hodlinator commented on pull request "[IBD] specialize CheckBlock's input & coinbase checks":
(https://github.com/bitcoin/bitcoin/pull/31682#discussion_r1992337395)
This comment made me wonder if one could remove the loop, I think we could:
```diff
diff --git a/src/consensus/tx_check.cpp b/src/consensus/tx_check.cpp
index 0a471deda2..6aa8a73de6 100644
--- a/src/consensus/tx_check.cpp
+++ b/src/consensus/tx_check.cpp
@@ -57,16 +57,21 @@ bool CheckTransaction(const CTransaction& tx, TxValidationState& state)
for (const auto& txin : tx.vin) {
sortedPrevouts.push_back(txin.prevout);
}
- std::sort(sortedPrevouts.be
...
(https://github.com/bitcoin/bitcoin/pull/31682#discussion_r1992337395)
This comment made me wonder if one could remove the loop, I think we could:
```diff
diff --git a/src/consensus/tx_check.cpp b/src/consensus/tx_check.cpp
index 0a471deda2..6aa8a73de6 100644
--- a/src/consensus/tx_check.cpp
+++ b/src/consensus/tx_check.cpp
@@ -57,16 +57,21 @@ bool CheckTransaction(const CTransaction& tx, TxValidationState& state)
for (const auto& txin : tx.vin) {
sortedPrevouts.push_back(txin.prevout);
}
- std::sort(sortedPrevouts.be
...
💬 brunoerg commented on pull request "descriptor: check whitespace in keys within fragments":
(https://github.com/bitcoin/bitcoin/pull/31603#issuecomment-2719220144)
rfm?
(https://github.com/bitcoin/bitcoin/pull/31603#issuecomment-2719220144)
rfm?
📝 davidgumberg opened a pull request: "contrib: Fix `gen-bitcoin-conf.sh`"
(https://github.com/bitcoin/bitcoin/pull/32049)
In #31118, the format of bitcoind's `--help` output changed slightly in a way that breaks `gen-bitcoin-conf.sh`, modify the script to accomodate the new format, by starting after the line that says "Options:" and strip the `-help` option and its description from the output.
Before this PR, all options above `-help` were excluded from the example bitcoin.conf.
(https://github.com/bitcoin/bitcoin/pull/32049)
In #31118, the format of bitcoind's `--help` output changed slightly in a way that breaks `gen-bitcoin-conf.sh`, modify the script to accomodate the new format, by starting after the line that says "Options:" and strip the `-help` option and its description from the output.
Before this PR, all options above `-help` were excluded from the example bitcoin.conf.