💬 ryanofsky commented on pull request "Rework validation logic for assumeutxo":
(https://github.com/bitcoin/bitcoin/pull/27746#discussion_r1246822305)
In commit "test: Clear block index flags when testing snapshots" (e8168cd56c490609f59e1dc89add58b1be18b516)
Adding `BLOCK_ASSUMED_VALID` here seems surprising since I would expect `ActivateSnapshot` below to be setting `BLOCK_ASSUMED_VALID`, and comment above doesn't mention this flag. I think it would be good to drop this line or add a comment explaining why it is necessary.
(https://github.com/bitcoin/bitcoin/pull/27746#discussion_r1246822305)
In commit "test: Clear block index flags when testing snapshots" (e8168cd56c490609f59e1dc89add58b1be18b516)
Adding `BLOCK_ASSUMED_VALID` here seems surprising since I would expect `ActivateSnapshot` below to be setting `BLOCK_ASSUMED_VALID`, and comment above doesn't mention this flag. I think it would be good to drop this line or add a comment explaining why it is necessary.
💬 ryanofsky commented on pull request "Rework validation logic for assumeutxo":
(https://github.com/bitcoin/bitcoin/pull/27746#discussion_r1246844224)
In commit "Move block-storage-related logic to ChainstateManager" (034f920d1755cff752d46b089ad37bdd703bf896)
Note: This change to the `CheckBlockIndex()` call is temporary and will revert back in later commit fb9c405506d6cfb6896e21b79789d7b6e638f6f2 "Move CheckBlockIndex() from Chainstate to ChainstateManager" when it becomes a `ChainstateManager` method.
There is not a change in behavior here in this commit, because in both places where
`Chainstate::AcceptBlock` was called previously (in
...
(https://github.com/bitcoin/bitcoin/pull/27746#discussion_r1246844224)
In commit "Move block-storage-related logic to ChainstateManager" (034f920d1755cff752d46b089ad37bdd703bf896)
Note: This change to the `CheckBlockIndex()` call is temporary and will revert back in later commit fb9c405506d6cfb6896e21b79789d7b6e638f6f2 "Move CheckBlockIndex() from Chainstate to ChainstateManager" when it becomes a `ChainstateManager` method.
There is not a change in behavior here in this commit, because in both places where
`Chainstate::AcceptBlock` was called previously (in
...
💬 ryanofsky commented on pull request "Rework validation logic for assumeutxo":
(https://github.com/bitcoin/bitcoin/pull/27746#discussion_r1246903052)
In commit "Fix initialization of setBlockIndexCandidates when working with multiple chainstates" (c2bc5cfd9977838af334844675d77090fe9c3c74)
Can add a comment here explaining the logic? I guess the `pindex == GetSnapshotBaseBlock()` case needs to be a special case because the snapshot block may not have transactions?
Maybe also would be more efficient to call `ActiveSnapshotBase` instead of `GetSnapshotBaseBlock` here. I think I would move the commit introducing the `ActiveSnapshotBase` met
...
(https://github.com/bitcoin/bitcoin/pull/27746#discussion_r1246903052)
In commit "Fix initialization of setBlockIndexCandidates when working with multiple chainstates" (c2bc5cfd9977838af334844675d77090fe9c3c74)
Can add a comment here explaining the logic? I guess the `pindex == GetSnapshotBaseBlock()` case needs to be a special case because the snapshot block may not have transactions?
Maybe also would be more efficient to call `ActiveSnapshotBase` instead of `GetSnapshotBaseBlock` here. I think I would move the commit introducing the `ActiveSnapshotBase` met
...
💬 ryanofsky commented on pull request "Rework validation logic for assumeutxo":
(https://github.com/bitcoin/bitcoin/pull/27746#discussion_r1246842096)
In commit "Move block-storage-related logic to ChainstateManager" (034f920d1755cff752d46b089ad37bdd703bf896)
Notes: It seems logical to call `GetAll` for all chainstates and let the `TryAddBlockIndexCandidate` and `FindMostWorkChain` functions figure out how to use the block and which chain to attach it to.
There is a change in behavior here if there are multiple chainstates, because previously this would only add blocks to `setBlockIndexCandidates` for the active chain, and now it will ad
...
(https://github.com/bitcoin/bitcoin/pull/27746#discussion_r1246842096)
In commit "Move block-storage-related logic to ChainstateManager" (034f920d1755cff752d46b089ad37bdd703bf896)
Notes: It seems logical to call `GetAll` for all chainstates and let the `TryAddBlockIndexCandidate` and `FindMostWorkChain` functions figure out how to use the block and which chain to attach it to.
There is a change in behavior here if there are multiple chainstates, because previously this would only add blocks to `setBlockIndexCandidates` for the active chain, and now it will ad
...
💬 ryanofsky commented on pull request "Rework validation logic for assumeutxo":
(https://github.com/bitcoin/bitcoin/pull/27746#discussion_r1246868905)
In commit "Move block-storage-related logic to ChainstateManager" (034f920d1755cff752d46b089ad37bdd703bf896)
Do you think it would be good if the `IsInitialBlockDownload` method were moved to `ChainstateManager`. Is there any place it should be actually called on the background chainstate rather than the active chainstate? Could add a TODO comment if IsInitialBlockDownload method should move like other methods are moving.
(https://github.com/bitcoin/bitcoin/pull/27746#discussion_r1246868905)
In commit "Move block-storage-related logic to ChainstateManager" (034f920d1755cff752d46b089ad37bdd703bf896)
Do you think it would be good if the `IsInitialBlockDownload` method were moved to `ChainstateManager`. Is there any place it should be actually called on the background chainstate rather than the active chainstate? Could add a TODO comment if IsInitialBlockDownload method should move like other methods are moving.
💬 benthecarman commented on pull request "Allow configuring target block time for a signet":
(https://github.com/bitcoin/bitcoin/pull/27446#issuecomment-1619190944)
The wallet that I am working on supports and uses 0-conf channels. That doesn't negate the fact that shorter block times are useful.
(https://github.com/bitcoin/bitcoin/pull/27446#issuecomment-1619190944)
The wallet that I am working on supports and uses 0-conf channels. That doesn't negate the fact that shorter block times are useful.
💬 Xekyo commented on pull request "Bump unconfirmed ancestor transactions to target feerate":
(https://github.com/bitcoin/bitcoin/pull/26152#issuecomment-1619210922)
> Reading through recent comments, it seems somewhat expected that this version isn't yet fully working, let me know when you want me to run my tests again!
Sorry about that, I was a bit in a rush on Friday. I’ve fixed a number of issues, if this latest push’s checks turn up green, it might be more fruitful to try now.
(https://github.com/bitcoin/bitcoin/pull/26152#issuecomment-1619210922)
> Reading through recent comments, it seems somewhat expected that this version isn't yet fully working, let me know when you want me to run my tests again!
Sorry about that, I was a bit in a rush on Friday. I’ve fixed a number of issues, if this latest push’s checks turn up green, it might be more fruitful to try now.
💬 mzumsande commented on pull request "test: Use same timeout for all index sync":
(https://github.com/bitcoin/bitcoin/pull/27988#issuecomment-1619218007)
> Yeah, should be caused by an upstream CI change. I guess there should be no risk in changing 120s to 300s, even potentially fixing the issue you see.
Saw it on master now too: https://github.com/bitcoin/bitcoin/runs/14733904119
I think the reason why this didn't fail before is that the `coinstatsindex_unclean_shutdown` used `GetTime` / `NodeClock` instead of `SteadyClock` before this PR. So since mocktime was used (which doesn't change after the 100 blocks are mined), the timeout was
...
(https://github.com/bitcoin/bitcoin/pull/27988#issuecomment-1619218007)
> Yeah, should be caused by an upstream CI change. I guess there should be no risk in changing 120s to 300s, even potentially fixing the issue you see.
Saw it on master now too: https://github.com/bitcoin/bitcoin/runs/14733904119
I think the reason why this didn't fail before is that the `coinstatsindex_unclean_shutdown` used `GetTime` / `NodeClock` instead of `SteadyClock` before this PR. So since mocktime was used (which doesn't change after the 100 blocks are mined), the timeout was
...
📝 furszy opened a pull request: "test: bugfix, synchronize indexes synchronously"
(https://github.com/bitcoin/bitcoin/pull/28026)
Fixing the current CI failures.
The tests objective is to exercise the chain synchronization code,
verifying that the indexes are able to catch up with the chain-tip.
Therefore, there is no need to run the process on a worker thread.
Important Note:
After #27607, the `BaseIndex::Start` method will be almost empty.
Only containing the `ThreadSync` call, allowing us to extract the
thread ownership from the index class and place it into a thread pool
in the future (which is being intro
...
(https://github.com/bitcoin/bitcoin/pull/28026)
Fixing the current CI failures.
The tests objective is to exercise the chain synchronization code,
verifying that the indexes are able to catch up with the chain-tip.
Therefore, there is no need to run the process on a worker thread.
Important Note:
After #27607, the `BaseIndex::Start` method will be almost empty.
Only containing the `ThreadSync` call, allowing us to extract the
thread ownership from the index class and place it into a thread pool
in the future (which is being intro
...
🤔 furszy reviewed a pull request: "test: Use same timeout for all index sync"
(https://github.com/bitcoin/bitcoin/pull/27988#pullrequestreview-1511849302)
Made #28026 to abolish the timeout issues.
(https://github.com/bitcoin/bitcoin/pull/27988#pullrequestreview-1511849302)
Made #28026 to abolish the timeout issues.
✅ achow101 closed an issue: "wallets created on master get corrupted when processed with v25"
(https://github.com/bitcoin/bitcoin/issues/27915)
(https://github.com/bitcoin/bitcoin/issues/27915)
🚀 achow101 merged a pull request: "wallet: bugfix, always use apostrophe for spkm descriptor ID"
(https://github.com/bitcoin/bitcoin/pull/27920)
(https://github.com/bitcoin/bitcoin/pull/27920)
📝 achow101 opened a pull request: "test: Fixes and updates to wallet_backwards_compatibility.py for 25.0 and descriptor wallets"
(https://github.com/bitcoin/bitcoin/pull/28027)
It was somewhat surprising to me that wallet_backwards_compatibility.py did not catch #27915 since the purpose of the test is to find downgrade issues such as that. It turns out the test was deficient in several places when it came to testing descriptor wallets, as well as deficient in addition to failing to correctly test some releases.
This PR fixes these test cases, adds more informative logging, slightly refactors the entire test in order to better test future versions, and adds a 25.0 no
...
(https://github.com/bitcoin/bitcoin/pull/28027)
It was somewhat surprising to me that wallet_backwards_compatibility.py did not catch #27915 since the purpose of the test is to find downgrade issues such as that. It turns out the test was deficient in several places when it came to testing descriptor wallets, as well as deficient in addition to failing to correctly test some releases.
This PR fixes these test cases, adds more informative logging, slightly refactors the entire test in order to better test future versions, and adds a 25.0 no
...
💬 MarcoFalke commented on pull request "test: Fixes and updates to wallet_backwards_compatibility.py for 25.0 and descriptor wallets":
(https://github.com/bitcoin/bitcoin/pull/28027#discussion_r1251525740)
only if you re-touch, style nit in 95b140a4b75c61e4ae3f4d1ea6e2d4a6b9fa15e8 (and following commits): The `*_path` member properties can use the shorter `a / b / c` syntax, over `os.path.join(a, b, c)`.
(https://github.com/bitcoin/bitcoin/pull/28027#discussion_r1251525740)
only if you re-touch, style nit in 95b140a4b75c61e4ae3f4d1ea6e2d4a6b9fa15e8 (and following commits): The `*_path` member properties can use the shorter `a / b / c` syntax, over `os.path.join(a, b, c)`.
💬 MarcoFalke commented on pull request "test: bugfix, synchronize indexes synchronously":
(https://github.com/bitcoin/bitcoin/pull/28026#issuecomment-1619565625)
lgtm ACK f01cb3a8dcc12954d5b6e075ebeb94c00fd37779
(https://github.com/bitcoin/bitcoin/pull/28026#issuecomment-1619565625)
lgtm ACK f01cb3a8dcc12954d5b6e075ebeb94c00fd37779
💬 MarcoFalke commented on pull request "test: Fix intermittent issue in mining_getblocktemplate_longpoll.py":
(https://github.com/bitcoin/bitcoin/pull/27941#issuecomment-1619804135)
@luke-jr I presume if this was a bug on the RPC side, it could be fixed with something like:
```diff
diff --git a/src/rpc/mining.cpp b/src/rpc/mining.cpp
index 074cecadd2..ca7d1acda8 100644
--- a/src/rpc/mining.cpp
+++ b/src/rpc/mining.cpp
@@ -742,12 +742,12 @@ static RPCHelpMan getblocktemplate()
WAIT_LOCK(g_best_block_mutex, lock);
while (g_best_block == hashWatchedChain && IsRPCRunning())
{
+ // Check transactions for update
+
...
(https://github.com/bitcoin/bitcoin/pull/27941#issuecomment-1619804135)
@luke-jr I presume if this was a bug on the RPC side, it could be fixed with something like:
```diff
diff --git a/src/rpc/mining.cpp b/src/rpc/mining.cpp
index 074cecadd2..ca7d1acda8 100644
--- a/src/rpc/mining.cpp
+++ b/src/rpc/mining.cpp
@@ -742,12 +742,12 @@ static RPCHelpMan getblocktemplate()
WAIT_LOCK(g_best_block_mutex, lock);
while (g_best_block == hashWatchedChain && IsRPCRunning())
{
+ // Check transactions for update
+
...
👍 willcl-ark approved a pull request: "Support JSON-RPC 2.0 when requested by client"
(https://github.com/bitcoin/bitcoin/pull/27101#pullrequestreview-1511661787)
re-ACK 5f9179a
Verified that since my last review `git range-diff af86462e...5f9179a` comprised rebase-related changes. The CI failure is unrelated. Left a non-blocking comment.
(https://github.com/bitcoin/bitcoin/pull/27101#pullrequestreview-1511661787)
re-ACK 5f9179a
Verified that since my last review `git range-diff af86462e...5f9179a` comprised rebase-related changes. The CI failure is unrelated. Left a non-blocking comment.
💬 willcl-ark commented on pull request "Support JSON-RPC 2.0 when requested by client":
(https://github.com/bitcoin/bitcoin/pull/27101#discussion_r1251203825)
Just thinking out loud here; we use a code comment to indicate what the error code represents but in many of the other tests it appears we don't (admittedly here it is the point of the test, and in the other locations it's not). I wonder whether it might be clearer to have these enumerated in a single location, e.g. _util.py_, and then they could be used by name here and elsewhere? (this might be a change someone else will just make in the future otherwise, too).
I think it's probably better
...
(https://github.com/bitcoin/bitcoin/pull/27101#discussion_r1251203825)
Just thinking out loud here; we use a code comment to indicate what the error code represents but in many of the other tests it appears we don't (admittedly here it is the point of the test, and in the other locations it's not). I wonder whether it might be clearer to have these enumerated in a single location, e.g. _util.py_, and then they could be used by name here and elsewhere? (this might be a change someone else will just make in the future otherwise, too).
I think it's probably better
...
💬 ajtowns commented on pull request "[25.x] Parallel compact block downloads":
(https://github.com/bitcoin/bitcoin/pull/27752#issuecomment-1619965422)
ACK b8ad3220a9068f10c2b3b14b40f211372aeece31 ; confirmed patches are clean cherry-picks from master, and already tested patches prior to 25.0 release
(https://github.com/bitcoin/bitcoin/pull/27752#issuecomment-1619965422)
ACK b8ad3220a9068f10c2b3b14b40f211372aeece31 ; confirmed patches are clean cherry-picks from master, and already tested patches prior to 25.0 release
💬 stickies-v commented on issue "rpc: doc: RPCHelpMan missing request parameters for getblocktemplate":
(https://github.com/bitcoin/bitcoin/issues/27998#issuecomment-1619992173)
The description for "data" is wrong, copy-pasting from another (unrelated) data field is not a good approach here. I would suggest digging into the code to understand what "data" is used for, and/or reading the spec of [BIP22](https://github.com/bitcoin/bips/blob/master/bip-0022.mediawiki) and [BIP23](https://github.com/bitcoin/bips/blob/master/bip-0023.mediawiki)
(https://github.com/bitcoin/bitcoin/issues/27998#issuecomment-1619992173)
The description for "data" is wrong, copy-pasting from another (unrelated) data field is not a good approach here. I would suggest digging into the code to understand what "data" is used for, and/or reading the spec of [BIP22](https://github.com/bitcoin/bips/blob/master/bip-0022.mediawiki) and [BIP23](https://github.com/bitcoin/bips/blob/master/bip-0023.mediawiki)