💬 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";
| ^~~~~~~~~
...
💬 ismaelsadeeq commented on pull request "doc: warn that CheckBlock() underestimates sigops":
(https://github.com/bitcoin/bitcoin/pull/31624#discussion_r2087205482)
For me, comments are most useful in the following cases:
1. When the code is confusing or difficult to understand in such cases, a high-level overview is helpful, especially when the code cannot be simplified.
2. To provide justification for a function call or explain the rationale behind a particular decision.
3. To offer warnings, notes, or contextual insights for the reader I actually enjoy reading these kinds of comments, and I’ve noticed this is done in some places in the codebase.
4
...
(https://github.com/bitcoin/bitcoin/pull/31624#discussion_r2087205482)
For me, comments are most useful in the following cases:
1. When the code is confusing or difficult to understand in such cases, a high-level overview is helpful, especially when the code cannot be simplified.
2. To provide justification for a function call or explain the rationale behind a particular decision.
3. To offer warnings, notes, or contextual insights for the reader I actually enjoy reading these kinds of comments, and I’ve noticed this is done in some places in the codebase.
4
...
💬 hebasto commented on pull request "qt: 29.0 translations update":
(https://github.com/bitcoin/bitcoin/pull/32004#issuecomment-2877201167)
> 3. The translations for the following languages, which appear to be the result of a mistake or an act of vandalism, have been discarded:
>
> * Czech (cs)
> * Danish (da)
> * Dutch (nl)
I've restored most of damaged strings for these languages on Transifex.
(https://github.com/bitcoin/bitcoin/pull/32004#issuecomment-2877201167)
> 3. The translations for the following languages, which appear to be the result of a mistake or an act of vandalism, have been discarded:
>
> * Czech (cs)
> * Danish (da)
> * Dutch (nl)
I've restored most of damaged strings for these languages on Transifex.
👍 ryanofsky approved a pull request: "wallet: Ensure best block matches wallet scan state"
(https://github.com/bitcoin/bitcoin/pull/30221#pullrequestreview-2837244742)
Code review ACK e2e9c30a4d04bd8173c3386c5b97dbac227e810e just moving WriteBestBlock call from destructor back to RemoveWallet() since last review.
I left a bunch of suggestions below and repeated some of Martin's but they are all nitpicking at this point so feel free to ignore
(https://github.com/bitcoin/bitcoin/pull/30221#pullrequestreview-2837244742)
Code review ACK e2e9c30a4d04bd8173c3386c5b97dbac227e810e just moving WriteBestBlock call from destructor back to RemoveWallet() since last review.
I left a bunch of suggestions below and repeated some of Martin's but they are all nitpicking at this point so feel free to ignore
💬 ryanofsky commented on pull request "wallet: Ensure best block matches wallet scan state":
(https://github.com/bitcoin/bitcoin/pull/30221#discussion_r2087136318)
In commit "wallet: Update best block record after block dis/connect" (f1f254f6c9d3cead617300367442c1bbb449af7c)
re: https://github.com/bitcoin/bitcoin/pull/30221#discussion_r1947184556
> To deal with unclean shutdowns, during disconnect the reverse order compared to blockConnected would be better: If the locator is updated first, an unclean shutdown will lead to a rescan of the disconnected block (which might do nothing but correct the locator). But if txns are set to Inactive first and ha
...
(https://github.com/bitcoin/bitcoin/pull/30221#discussion_r2087136318)
In commit "wallet: Update best block record after block dis/connect" (f1f254f6c9d3cead617300367442c1bbb449af7c)
re: https://github.com/bitcoin/bitcoin/pull/30221#discussion_r1947184556
> To deal with unclean shutdowns, during disconnect the reverse order compared to blockConnected would be better: If the locator is updated first, an unclean shutdown will lead to a rescan of the disconnected block (which might do nothing but correct the locator). But if txns are set to Inactive first and ha
...
💬 ryanofsky commented on pull request "wallet: Ensure best block matches wallet scan state":
(https://github.com/bitcoin/bitcoin/pull/30221#discussion_r2087200068)
In commit "wallet: Replace chainStateFlushed in loading with SetLastBlockProcessed" (357a65fd426f2122f390803d572fd89d4b0177c4)
I think everything in the comment is accurate but also vague, and it raises questions about when the last scanned block would not match the last/best blocks. I think it might be better to be more concrete like:
// Set last block scanned as the last block processed, because it may be different in the case of a reorg. Also save the best block locator because rescanni
...
(https://github.com/bitcoin/bitcoin/pull/30221#discussion_r2087200068)
In commit "wallet: Replace chainStateFlushed in loading with SetLastBlockProcessed" (357a65fd426f2122f390803d572fd89d4b0177c4)
I think everything in the comment is accurate but also vague, and it raises questions about when the last scanned block would not match the last/best blocks. I think it might be better to be more concrete like:
// Set last block scanned as the last block processed, because it may be different in the case of a reorg. Also save the best block locator because rescanni
...
💬 ryanofsky commented on pull request "wallet: Ensure best block matches wallet scan state":
(https://github.com/bitcoin/bitcoin/pull/30221#discussion_r2087106482)
In commit "wallet: Update best block record after block dis/connect" (f1f254f6c9d3cead617300367442c1bbb449af7c)
EDIT: Martin left a similar suggestion https://github.com/bitcoin/bitcoin/pull/30221#discussion_r2085051375. Would be nice to follow up on any of these ideas.
---
It seems like this code is redundant and could be simplified:
```diff
--- a/src/wallet/wallet.cpp
+++ b/src/wallet/wallet.cpp
@@ -666,12 +666,7 @@ void CWallet::SetLastBlockProcessed(int block_height, uint256 b
...
(https://github.com/bitcoin/bitcoin/pull/30221#discussion_r2087106482)
In commit "wallet: Update best block record after block dis/connect" (f1f254f6c9d3cead617300367442c1bbb449af7c)
EDIT: Martin left a similar suggestion https://github.com/bitcoin/bitcoin/pull/30221#discussion_r2085051375. Would be nice to follow up on any of these ideas.
---
It seems like this code is redundant and could be simplified:
```diff
--- a/src/wallet/wallet.cpp
+++ b/src/wallet/wallet.cpp
@@ -666,12 +666,7 @@ void CWallet::SetLastBlockProcessed(int block_height, uint256 b
...
💬 ryanofsky commented on pull request "wallet: Ensure best block matches wallet scan state":
(https://github.com/bitcoin/bitcoin/pull/30221#discussion_r2087113393)
re: https://github.com/bitcoin/bitcoin/pull/30221#discussion_r2085053587
In commit "wallet: Update best block record after block dis/connect" (f1f254f6c9d3cead617300367442c1bbb449af7c)
> nit: could also call just introduce and use `WriteBestBlock()` here since `SetLastBlockProcessedInMem` is already called above.
This does seem like a nice suggestion. I also wonder if we can consolidate the seperate updates in blockConnected, deleting SetLastBlockProcessedInMem at the top, and moving it
...
(https://github.com/bitcoin/bitcoin/pull/30221#discussion_r2087113393)
re: https://github.com/bitcoin/bitcoin/pull/30221#discussion_r2085053587
In commit "wallet: Update best block record after block dis/connect" (f1f254f6c9d3cead617300367442c1bbb449af7c)
> nit: could also call just introduce and use `WriteBestBlock()` here since `SetLastBlockProcessedInMem` is already called above.
This does seem like a nice suggestion. I also wonder if we can consolidate the seperate updates in blockConnected, deleting SetLastBlockProcessedInMem at the top, and moving it
...
💬 ryanofsky commented on pull request "wallet: Ensure best block matches wallet scan state":
(https://github.com/bitcoin/bitcoin/pull/30221#discussion_r2087237457)
In commit "wallet: Write best block record on unload" (8ecc7653556a1977eae5a902d51a2ececde94913)
Just to be sure, current state is that if WriteBestBlock is called here, rather than being called in the destructor or being called in FlushAndDeleteWallet, the best block locator could be a little out of date and this is maybe not ideal but fine?
re: https://github.com/bitcoin/bitcoin/pull/30221#issuecomment-2864638615
> It can't be static because the `FUZZ_TARGET` below needs to access it.
...
(https://github.com/bitcoin/bitcoin/pull/30221#discussion_r2087237457)
In commit "wallet: Write best block record on unload" (8ecc7653556a1977eae5a902d51a2ececde94913)
Just to be sure, current state is that if WriteBestBlock is called here, rather than being called in the destructor or being called in FlushAndDeleteWallet, the best block locator could be a little out of date and this is maybe not ideal but fine?
re: https://github.com/bitcoin/bitcoin/pull/30221#issuecomment-2864638615
> It can't be static because the `FUZZ_TARGET` below needs to access it.
...