🤔 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.
💬 l0rinc commented on pull request "[IBD] specialize CheckBlock's input & coinbase checks":
(https://github.com/bitcoin/bitcoin/pull/31682#discussion_r1992367875)
Thanks for the hint, this is also a good alternative.
That combination is tested via:
https://github.com/bitcoin/bitcoin/blob/6421b986f6c043fd1266c399e542279ad373822f/src/test/transaction_tests.cpp#L504
I ran `CheckBlockBench` a few times to see if there's any difference and there's barely any (the current one seemed a tiny bit faster with Clang) - so I'd rather go for simpler code.
In reality though the loop exits immediately since we rarely have hashes of 0, so the two solutions are
...
(https://github.com/bitcoin/bitcoin/pull/31682#discussion_r1992367875)
Thanks for the hint, this is also a good alternative.
That combination is tested via:
https://github.com/bitcoin/bitcoin/blob/6421b986f6c043fd1266c399e542279ad373822f/src/test/transaction_tests.cpp#L504
I ran `CheckBlockBench` a few times to see if there's any difference and there's barely any (the current one seemed a tiny bit faster with Clang) - so I'd rather go for simpler code.
In reality though the loop exits immediately since we rarely have hashes of 0, so the two solutions are
...
💬 davidgumberg commented on pull request "[29.x] bump to v29.0rc1":
(https://github.com/bitcoin/bitcoin/pull/32046#issuecomment-2719259291)
It appears that https://github.com/bitcoin/bitcoin/pull/31118 broke the `bitcoin.conf` generating script, all options above `-help` in the `--help` output are missing, including e.g. `-dbcache`. I've opened a fix in #32049.
<details>
<summary>
### bitcoin.conf diff between 28.1 and #32046
</summary>
```console
$ git diff v28.1/share/examples/bitcoin.conf 32046/share/examples/bitcoin.conf
```
```diff
diff --git a/v28.1/share/examples/bitcoin.conf b/32046/share/examples/bitcoi
...
(https://github.com/bitcoin/bitcoin/pull/32046#issuecomment-2719259291)
It appears that https://github.com/bitcoin/bitcoin/pull/31118 broke the `bitcoin.conf` generating script, all options above `-help` in the `--help` output are missing, including e.g. `-dbcache`. I've opened a fix in #32049.
<details>
<summary>
### bitcoin.conf diff between 28.1 and #32046
</summary>
```console
$ git diff v28.1/share/examples/bitcoin.conf 32046/share/examples/bitcoin.conf
```
```diff
diff --git a/v28.1/share/examples/bitcoin.conf b/32046/share/examples/bitcoi
...
💬 Nouridin commented on issue "BnB untested/unused condition in UTXO exclusion optimization":
(https://github.com/bitcoin/bitcoin/issues/32047#issuecomment-2719272219)
In summary, while the fee check may be redundant given the current relationships between effective value and fee, it was likely originally added to ensure that the shortcut is _only_ applied when both the effective value and the spending fee are identical. Since testing shows that removing it does not lead to failures, it appears safe to remove it, but any change in this well‐tuned coin selection code must be approached with caution given the subtle and sometimes unexpected interactions in coin
...
(https://github.com/bitcoin/bitcoin/issues/32047#issuecomment-2719272219)
In summary, while the fee check may be redundant given the current relationships between effective value and fee, it was likely originally added to ensure that the shortcut is _only_ applied when both the effective value and the spending fee are identical. Since testing shows that removing it does not lead to failures, it appears safe to remove it, but any change in this well‐tuned coin selection code must be approached with caution given the subtle and sometimes unexpected interactions in coin
...
💬 davidgumberg commented on pull request "ci: Test cross-built Windows executables on Windows natively":
(https://github.com/bitcoin/bitcoin/pull/31176#issuecomment-2719278848)
> It's true that wine tests can't catch every bug that could happen on windows since Wine is another platform and "Is Not an Emulator." But calling the wine tests "fundamentally broken" would seem to imply that there were bugs in wine or in the wine test setup, and I haven't seen evidence for that. It seems more likely to me that some of our own tests are flaky and not reliable or portable. At least I didn't see any evidence that the wine setup was actually broken when I asked about this previou
...
(https://github.com/bitcoin/bitcoin/pull/31176#issuecomment-2719278848)
> It's true that wine tests can't catch every bug that could happen on windows since Wine is another platform and "Is Not an Emulator." But calling the wine tests "fundamentally broken" would seem to imply that there were bugs in wine or in the wine test setup, and I haven't seen evidence for that. It seems more likely to me that some of our own tests are flaky and not reliable or portable. At least I didn't see any evidence that the wine setup was actually broken when I asked about this previou
...
💬 hodlinator commented on pull request "refactor: CLI cleanups":
(https://github.com/bitcoin/bitcoin/pull/31887#discussion_r1992357083)
info: The `NetworkStringToId()` duplication was introduced in db4d2c282afd46709792aaf2d36ffbfc1745b776. Could add a reference in the commit message if you retouch.
(https://github.com/bitcoin/bitcoin/pull/31887#discussion_r1992357083)
info: The `NetworkStringToId()` duplication was introduced in db4d2c282afd46709792aaf2d36ffbfc1745b776. Could add a reference in the commit message if you retouch.
👍 hodlinator approved a pull request: "refactor: CLI cleanups"
(https://github.com/bitcoin/bitcoin/pull/31887#pullrequestreview-2679972740)
Code review ACK d423fd9ec83ae323ac29bdc1b677ed8260bd59c4
Good to see `-getinfo` negation behavior fixed. I encountered it when exploring args in https://github.com/bitcoin/bitcoin/pull/16545#pullrequestreview-2316714013 (in the "Expand/collapse lengthy exploration"-section).
Still think the 3rd commit is out of character compared to my brief experience with this project as it is purely stylistic, but at least the formatting was [fixed](https://github.com/bitcoin/bitcoin/pull/31887#discussi
...
(https://github.com/bitcoin/bitcoin/pull/31887#pullrequestreview-2679972740)
Code review ACK d423fd9ec83ae323ac29bdc1b677ed8260bd59c4
Good to see `-getinfo` negation behavior fixed. I encountered it when exploring args in https://github.com/bitcoin/bitcoin/pull/16545#pullrequestreview-2316714013 (in the "Expand/collapse lengthy exploration"-section).
Still think the 3rd commit is out of character compared to my brief experience with this project as it is purely stylistic, but at least the formatting was [fixed](https://github.com/bitcoin/bitcoin/pull/31887#discussi
...
💬 hodlinator commented on pull request "refactor: CLI cleanups":
(https://github.com/bitcoin/bitcoin/pull/31887#discussion_r1992388251)
Thread https://github.com/bitcoin/bitcoin/pull/31887#discussion_r1960004078:
I still read https://github.com/bitcoin/bitcoin/pull/31887#discussion_r1966566982 as *what can be done* technically, not *what should be done* (convention). What comes close to indicating convention is the SO-link which shows that some random guy on the internet also prioritizes default visibility over other conventions. His example of taking a `struct` from a C library and inheriting from it into a `class` is far fr
...
(https://github.com/bitcoin/bitcoin/pull/31887#discussion_r1992388251)
Thread https://github.com/bitcoin/bitcoin/pull/31887#discussion_r1960004078:
I still read https://github.com/bitcoin/bitcoin/pull/31887#discussion_r1966566982 as *what can be done* technically, not *what should be done* (convention). What comes close to indicating convention is the SO-link which shows that some random guy on the internet also prioritizes default visibility over other conventions. His example of taking a `struct` from a C library and inheriting from it into a `class` is far fr
...
💬 hodlinator commented on pull request "refactor: CLI cleanups":
(https://github.com/bitcoin/bitcoin/pull/31887#discussion_r1992361754)
nit: If you re-touch it might be good to add the missing `#` to the commit message (for tooling/consistency). Unless "PR <number>" is also a convention for tooling unbeknownst to me.
```diff
-PR 21192
+PR #21192
```
(https://github.com/bitcoin/bitcoin/pull/31887#discussion_r1992361754)
nit: If you re-touch it might be good to add the missing `#` to the commit message (for tooling/consistency). Unless "PR <number>" is also a convention for tooling unbeknownst to me.
```diff
-PR 21192
+PR #21192
```
💬 hodlinator commented on pull request "[IBD] specialize CheckBlock's input & coinbase checks":
(https://github.com/bitcoin/bitcoin/pull/31682#discussion_r1992427691)
> I ran `CheckBlockBench` a few times to see if there's any difference and there's barely any (the current one seemed a tiny bit faster with Clang) - so I'd rather go for simpler code.
I have yet to have a run where my alternative was not faster when I set `-min-time=30000`. Seems to be around 2% faster on my CPU.
Agree that it's good to not complicate consensus code. As you say the loop exits immediately after initializing the iterator and checking that it's not at the end, then checking
...
(https://github.com/bitcoin/bitcoin/pull/31682#discussion_r1992427691)
> I ran `CheckBlockBench` a few times to see if there's any difference and there's barely any (the current one seemed a tiny bit faster with Clang) - so I'd rather go for simpler code.
I have yet to have a run where my alternative was not faster when I set `-min-time=30000`. Seems to be around 2% faster on my CPU.
Agree that it's good to not complicate consensus code. As you say the loop exits immediately after initializing the iterator and checking that it's not at the end, then checking
...
💬 achow101 commented on pull request "[29.x] bump to v29.0rc1":
(https://github.com/bitcoin/bitcoin/pull/32046#issuecomment-2719368979)
ACK 47e2fa86dc5433852fd9e5050a23de2accfdca8
Other than the issue with the example bitcoin.conf, everything seems correct. I think we can move forward with rc1 and just backport the gen-bitcoin-conf.sh fixes for rc2 (or final). The script has to be rerun for every rc anyways.
(https://github.com/bitcoin/bitcoin/pull/32046#issuecomment-2719368979)
ACK 47e2fa86dc5433852fd9e5050a23de2accfdca8
Other than the issue with the example bitcoin.conf, everything seems correct. I think we can move forward with rc1 and just backport the gen-bitcoin-conf.sh fixes for rc2 (or final). The script has to be rerun for every rc anyways.
✅ fanquake closed a pull request: "docs: Enhance TODO section in README.md with detailed explanations"
(https://github.com/bitcoin/bitcoin/pull/32048)
(https://github.com/bitcoin/bitcoin/pull/32048)
💬 jonatack commented on pull request "refactor: CLI cleanups":
(https://github.com/bitcoin/bitcoin/pull/31887#discussion_r1992475317)
I omit the `#` in cases like this, where it isn't really important to link them here, so that GitHub does not add a notification in that PR every time this branch is (re)pushed.
(https://github.com/bitcoin/bitcoin/pull/31887#discussion_r1992475317)
I omit the `#` in cases like this, where it isn't really important to link them here, so that GitHub does not add a notification in that PR every time this branch is (re)pushed.
💬 jonatack commented on pull request "refactor: CLI cleanups":
(https://github.com/bitcoin/bitcoin/pull/31887#discussion_r1992477649)
Yes, here it seems cleaner, fitting the use case and requiring less code to do the same thing. I appreciate your indulgence here.
(https://github.com/bitcoin/bitcoin/pull/31887#discussion_r1992477649)
Yes, here it seems cleaner, fitting the use case and requiring less code to do the same thing. I appreciate your indulgence here.
📝 theStack opened a pull request: "test: avoid treating hash results as integers (part 1)"
(https://github.com/bitcoin/bitcoin/pull/32050)
In the functional test framework we currently have a strong tendency to treat and store identifiers that result from hash functions (e.g. (w)txids, block hashes) as integers, which seems an unnatural and confusing choice. Hashes are just pseudo-random sequences of bytes, and it almost never makes sense to apply integer operations on them; the only exceptions I could think of is PoW-verification of block hashes with the less-than (`<`) operator, or interpreting the byte-string as scalar in the EC
...
(https://github.com/bitcoin/bitcoin/pull/32050)
In the functional test framework we currently have a strong tendency to treat and store identifiers that result from hash functions (e.g. (w)txids, block hashes) as integers, which seems an unnatural and confusing choice. Hashes are just pseudo-random sequences of bytes, and it almost never makes sense to apply integer operations on them; the only exceptions I could think of is PoW-verification of block hashes with the less-than (`<`) operator, or interpreting the byte-string as scalar in the EC
...