💬 bytejedi commented on pull request "policy: uncap datacarrier by default":
(https://github.com/bitcoin/bitcoin/pull/32406#issuecomment-2877049144)
Bitcoin is a digital currency that OP_RETURN should NOT EXIST ever since the beginning. You all turning bitcoin to shitcoin. Look what you all did these years, just fix bugs and CVEs please.
(https://github.com/bitcoin/bitcoin/pull/32406#issuecomment-2877049144)
Bitcoin is a digital currency that OP_RETURN should NOT EXIST ever since the beginning. You all turning bitcoin to shitcoin. Look what you all did these years, just fix bugs and CVEs please.
💬 maflcko commented on pull request "Update `secp256k1` subtree to latest master":
(https://github.com/bitcoin/bitcoin/pull/32028#issuecomment-2877052289)
> I can't seem to find `CHECK(secp256k1_ec_seckey_tweak_add(CTX, privkey, rnd)` in either our, or libsecp256k1s codebase.
See https://github.com/maflcko/DrahtBot/issues/50.
The prior result was correct, but it (obviously) can't be fixed here in a subtree bump anyway:
- dependendencies -> dependencies [typo]
- uninitalized -> uninitialized [typo]
- lamba -> lambda [typo]
(https://github.com/bitcoin/bitcoin/pull/32028#issuecomment-2877052289)
> I can't seem to find `CHECK(secp256k1_ec_seckey_tweak_add(CTX, privkey, rnd)` in either our, or libsecp256k1s codebase.
See https://github.com/maflcko/DrahtBot/issues/50.
The prior result was correct, but it (obviously) can't be fixed here in a subtree bump anyway:
- dependendencies -> dependencies [typo]
- uninitalized -> uninitialized [typo]
- lamba -> lambda [typo]
🤔 stickies-v reviewed a pull request: "kernel: Separate UTXO set access from validation functions"
(https://github.com/bitcoin/bitcoin/pull/32317#pullrequestreview-2802813202)
Concept ACK
Reducing the coupling between validation functions and the UTXO set makes the validation functions more usable, and more testable. The approach taken here seems sensible, but I'm going to think about alternatives a bit more.
(https://github.com/bitcoin/bitcoin/pull/32317#pullrequestreview-2802813202)
Concept ACK
Reducing the coupling between validation functions and the UTXO set makes the validation functions more usable, and more testable. The approach taken here seems sensible, but I'm going to think about alternatives a bit more.
💬 stickies-v commented on pull request "kernel: Separate UTXO set access from validation functions":
(https://github.com/bitcoin/bitcoin/pull/32317#discussion_r2066960211)
I presume adding `block_hash` is done to avoid calculating `CBlockHeader::GetHash()` multiple times? Did you find that to have a measurable impact somewhere?
I think the most elegant approach would be to implement caching inside `CBlockHeader::GetHash()` (if meaningful), but that would involve a pretty big LoC change making the `CBlockHeader` members private and adding getters.
If we stick with this approach, I think at least it should be documented that `block_hash` is duplicated for perf
...
(https://github.com/bitcoin/bitcoin/pull/32317#discussion_r2066960211)
I presume adding `block_hash` is done to avoid calculating `CBlockHeader::GetHash()` multiple times? Did you find that to have a measurable impact somewhere?
I think the most elegant approach would be to implement caching inside `CBlockHeader::GetHash()` (if meaningful), but that would involve a pretty big LoC change making the `CBlockHeader` members private and adding getters.
If we stick with this approach, I think at least it should be documented that `block_hash` is duplicated for perf
...
💬 stickies-v commented on pull request "kernel: Separate UTXO set access from validation functions":
(https://github.com/bitcoin/bitcoin/pull/32317#discussion_r2075811232)
Since `std::reference_wrapper<T>` implicitly converts to `T&`, I don't think we actually need this helper? At least it compiles fine on my machine. If you've found any compiler issues, perhaps documenting them would be good so we can remove it in the future?
<details>
<summary>git diff on 76a8f22c5c</summary>
```diff
diff --git a/src/consensus/tx_verify.cpp b/src/consensus/tx_verify.cpp
index 97f8241089..f2a0259b23 100644
--- a/src/consensus/tx_verify.cpp
+++ b/src/consensus/tx_verify
...
(https://github.com/bitcoin/bitcoin/pull/32317#discussion_r2075811232)
Since `std::reference_wrapper<T>` implicitly converts to `T&`, I don't think we actually need this helper? At least it compiles fine on my machine. If you've found any compiler issues, perhaps documenting them would be good so we can remove it in the future?
<details>
<summary>git diff on 76a8f22c5c</summary>
```diff
diff --git a/src/consensus/tx_verify.cpp b/src/consensus/tx_verify.cpp
index 97f8241089..f2a0259b23 100644
--- a/src/consensus/tx_verify.cpp
+++ b/src/consensus/tx_verify
...
💬 stickies-v commented on pull request "kernel: Separate UTXO set access from validation functions":
(https://github.com/bitcoin/bitcoin/pull/32317#discussion_r2065965592)
nit: I think this is an `@param[in,out]`?
(https://github.com/bitcoin/bitcoin/pull/32317#discussion_r2065965592)
nit: I think this is an `@param[in,out]`?
💬 stickies-v commented on pull request "kernel: Separate UTXO set access from validation functions":
(https://github.com/bitcoin/bitcoin/pull/32317#discussion_r2075895517)
nit: for non-trivial functions, I think it helps to narrow down `T` instead of having to deduct it from its callsites and usage.
What do you think about adding an `IsCoinRef` concept, and adding `std::span` to the function signature?
<details>
<summary>git diff on 76a8f22c5c</summary>
```diff
diff --git a/src/consensus/tx_verify.cpp b/src/consensus/tx_verify.cpp
index 97f8241089..ab27387cc4 100644
--- a/src/consensus/tx_verify.cpp
+++ b/src/consensus/tx_verify.cpp
@@ -14,6 +14,8 @
...
(https://github.com/bitcoin/bitcoin/pull/32317#discussion_r2075895517)
nit: for non-trivial functions, I think it helps to narrow down `T` instead of having to deduct it from its callsites and usage.
What do you think about adding an `IsCoinRef` concept, and adding `std::span` to the function signature?
<details>
<summary>git diff on 76a8f22c5c</summary>
```diff
diff --git a/src/consensus/tx_verify.cpp b/src/consensus/tx_verify.cpp
index 97f8241089..ab27387cc4 100644
--- a/src/consensus/tx_verify.cpp
+++ b/src/consensus/tx_verify.cpp
@@ -14,6 +14,8 @
...
💬 stickies-v commented on pull request "kernel: Separate UTXO set access from validation functions":
(https://github.com/bitcoin/bitcoin/pull/32317#discussion_r2077901694)
I think you can use `AccessCoins` here?
```suggestion
for (const Coin& coin : AccessCoins(tx)) {
assert(!coin.IsSpent());
spent_outputs.emplace_back(coin.out);
}
```
(https://github.com/bitcoin/bitcoin/pull/32317#discussion_r2077901694)
I think you can use `AccessCoins` here?
```suggestion
for (const Coin& coin : AccessCoins(tx)) {
assert(!coin.IsSpent());
spent_outputs.emplace_back(coin.out);
}
```
💬 stickies-v commented on pull request "kernel: Separate UTXO set access from validation functions":
(https://github.com/bitcoin/bitcoin/pull/32317#discussion_r2080154034)
```suggestion
* undo data to disk, and raises the block validity in the block index.
```
(https://github.com/bitcoin/bitcoin/pull/32317#discussion_r2080154034)
```suggestion
* undo data to disk, and raises the block validity in the block index.
```
💬 stickies-v commented on pull request "kernel: Separate UTXO set access from validation functions":
(https://github.com/bitcoin/bitcoin/pull/32317#discussion_r2086974181)
This is UB if e.g. blockundo is not populated, would add a size assertion at the beginning:
<details>
<summary>git diff on 16a695fbff</summary>
```diff
diff --git a/src/validation.cpp b/src/validation.cpp
index adec151e14..20e1dc6482 100644
--- a/src/validation.cpp
+++ b/src/validation.cpp
@@ -2446,6 +2446,7 @@ bool Chainstate::ConnectBlock(const CBlock& block, const uint256& block_hash, co
assert(pindex);
Assume(block.GetHash() == block_hash);
assert(*pindex->phash
...
(https://github.com/bitcoin/bitcoin/pull/32317#discussion_r2086974181)
This is UB if e.g. blockundo is not populated, would add a size assertion at the beginning:
<details>
<summary>git diff on 16a695fbff</summary>
```diff
diff --git a/src/validation.cpp b/src/validation.cpp
index adec151e14..20e1dc6482 100644
--- a/src/validation.cpp
+++ b/src/validation.cpp
@@ -2446,6 +2446,7 @@ bool Chainstate::ConnectBlock(const CBlock& block, const uint256& block_hash, co
assert(pindex);
Assume(block.GetHash() == block_hash);
assert(*pindex->phash
...
💬 stickies-v commented on pull request "kernel: Separate UTXO set access from validation functions":
(https://github.com/bitcoin/bitcoin/pull/32317#discussion_r2086983097)
```suggestion
* @param[out] state This is set to an Error state if any error occurred while validating block
```
(https://github.com/bitcoin/bitcoin/pull/32317#discussion_r2086983097)
```suggestion
* @param[out] state This is set to an Error state if any error occurred while validating block
```
💬 stickies-v commented on pull request "kernel: Separate UTXO set access from validation functions":
(https://github.com/bitcoin/bitcoin/pull/32317#discussion_r2086978979)
> Validity checks that depend on the UTXO set are also done
nit: I don't think that's correct anymore?
(https://github.com/bitcoin/bitcoin/pull/32317#discussion_r2086978979)
> Validity checks that depend on the UTXO set are also done
nit: I don't think that's correct anymore?
💬 stickies-v commented on pull request "policy: uncap datacarrier by default":
(https://github.com/bitcoin/bitcoin/pull/32406#discussion_r2087059998)
I just mean adding something like `assert_greater_than(len(b'\xff' * 5_000), MAX_SCRIPT_ELEMENT_SIZE)`. Adding a docstring that is not actually enforced in tests is confusing and fragile imo. Unless this is already tested in some other way I'm missing? If it shouldn't be tested, it probably shouldn't be documented either?
(https://github.com/bitcoin/bitcoin/pull/32406#discussion_r2087059998)
I just mean adding something like `assert_greater_than(len(b'\xff' * 5_000), MAX_SCRIPT_ELEMENT_SIZE)`. Adding a docstring that is not actually enforced in tests is confusing and fragile imo. Unless this is already tested in some other way I'm missing? If it shouldn't be tested, it probably shouldn't be documented either?
👍 stickies-v approved a pull request: "policy: uncap datacarrier by default"
(https://github.com/bitcoin/bitcoin/pull/32406#pullrequestreview-2837165602)
re-ACK 35bcd8eed38130445aef5ebe217ab42248fa6f18
No changes except addressing review feedback:
- squashing commits
- improving release notes
- test style changes
> Smaller code code changes can be left to followups, as it's tedious to keep track of actual code review comments in this PR due to continued brigading.
Agreed. Leaving nits here just for reference but they shouldn't block progress.
(https://github.com/bitcoin/bitcoin/pull/32406#pullrequestreview-2837165602)
re-ACK 35bcd8eed38130445aef5ebe217ab42248fa6f18
No changes except addressing review feedback:
- squashing commits
- improving release notes
- test style changes
> Smaller code code changes can be left to followups, as it's tedious to keep track of actual code review comments in this PR due to continued brigading.
Agreed. Leaving nits here just for reference but they shouldn't block progress.
👋 Eunovo's pull request is ready for review: "Fix listdescriptors true fails with 'Can't get descriptor string' in non-watch-only descriptor wallet"
(https://github.com/bitcoin/bitcoin/pull/32471)
(https://github.com/bitcoin/bitcoin/pull/32471)
👍 theStack approved a pull request: "signet: omit commitment for some trivial challenges"
(https://github.com/bitcoin/bitcoin/pull/29032#pullrequestreview-2837357698)
re-ACK 2be93f7ba3866892d98795384779ce2e965753a1
(https://github.com/bitcoin/bitcoin/pull/29032#pullrequestreview-2837357698)
re-ACK 2be93f7ba3866892d98795384779ce2e965753a1
💬 Eunovo commented on pull request "Fix listdescriptors true fails with 'Can't get descriptor string' in non-watch-only descriptor wallet":
(https://github.com/bitcoin/bitcoin/pull/32471#issuecomment-2877123412)
cc @rkrux @sipa
(https://github.com/bitcoin/bitcoin/pull/32471#issuecomment-2877123412)
cc @rkrux @sipa
💬 kevkevinpal commented on pull request "test: added fuzz coverage for consensus/merkle.cpp":
(https://github.com/bitcoin/bitcoin/pull/32243#discussion_r2087175769)
I pushed [f99fd0b](https://github.com/bitcoin/bitcoin/pull/32243/commits/f99fd0b87dcfaf84784ce423f78a950ad377b36b) which now uses `ConsumeDeserializable`
I haven't used `ConsumeDeserializable` so I'm not 100% sure if I'm using it correctly.
(https://github.com/bitcoin/bitcoin/pull/32243#discussion_r2087175769)
I pushed [f99fd0b](https://github.com/bitcoin/bitcoin/pull/32243/commits/f99fd0b87dcfaf84784ce423f78a950ad377b36b) which now uses `ConsumeDeserializable`
I haven't used `ConsumeDeserializable` so I'm not 100% sure if I'm using it correctly.
📝 mzumsande opened a pull request: "test: fix two intermittent timeouts in wallet_basic.py"
(https://github.com/bitcoin/bitcoin/pull/32483)
Fixes two rare timeouts that happened in the CI:
#27249:
There could be a race with outstanding TxAddedToMempool notifications being applied to the soon-to-be created wallet:
1. importdescriptors during rescan sets status to `TxStateConfirmed`
2. old `transactionAddedToMempool` notification changes status back to `TxStateInMempool`
3. If the listunspent call happens here the test will fail
4. blockConnected notification will change the status back to `TxStateConfirmed` (so it's not a pe
...
(https://github.com/bitcoin/bitcoin/pull/32483)
Fixes two rare timeouts that happened in the CI:
#27249:
There could be a race with outstanding TxAddedToMempool notifications being applied to the soon-to-be created wallet:
1. importdescriptors during rescan sets status to `TxStateConfirmed`
2. old `transactionAddedToMempool` notification changes status back to `TxStateInMempool`
3. If the listunspent call happens here the test will fail
4. blockConnected notification will change the status back to `TxStateConfirmed` (so it's not a pe
...
📝 fanquake opened a pull request: "[28.x] build: suppress `-Wunterminated-string-initialization` in secp256k1"
(https://github.com/bitcoin/bitcoin/pull/32484)
Suppress `-Wunterminated-string-initialization` warnings from GCC 15, like:
```bash
In file included from src/secp256k1.c:830:
src/modules/schnorrsig/main_impl.h:48:46: warning: initializer-string for array of 'unsigned char' truncates NUL terminator but destination lacks 'nonstring' attribute (14 chars into 13 available) [-Wunterminated-string-initialization]
48 | static const unsigned char bip340_algo[13] = "BIP0340/nonce";
| ^~~~~~~~~
...
(https://github.com/bitcoin/bitcoin/pull/32484)
Suppress `-Wunterminated-string-initialization` warnings from GCC 15, like:
```bash
In file included from src/secp256k1.c:830:
src/modules/schnorrsig/main_impl.h:48:46: warning: initializer-string for array of 'unsigned char' truncates NUL terminator but destination lacks 'nonstring' attribute (14 chars into 13 available) [-Wunterminated-string-initialization]
48 | static const unsigned char bip340_algo[13] = "BIP0340/nonce";
| ^~~~~~~~~
...