🤔 marcofleon reviewed a pull request: "fuzz: Make process_message(s) more deterministic"
(https://github.com/bitcoin/bitcoin/pull/32822#pullrequestreview-2980950526)
tACK fac673d434b4d32d8c44dcc50a3655ba4a1816de
Running the stability script, there's significantly less coverage differences with this PR. My only question is why does `ResetChainman` set mock time in `process_messages` but not in `process_message`? Didn't see an obvious reason, but I could be missing something.
(https://github.com/bitcoin/bitcoin/pull/32822#pullrequestreview-2980950526)
tACK fac673d434b4d32d8c44dcc50a3655ba4a1816de
Running the stability script, there's significantly less coverage differences with this PR. My only question is why does `ResetChainman` set mock time in `process_messages` but not in `process_message`? Didn't see an obvious reason, but I could be missing something.
💬 furszy commented on pull request "test: fix feature_init.py intermittencies":
(https://github.com/bitcoin/bitcoin/pull/32835#discussion_r2181163397)
that was actually done on purpose. See https://leimao.github.io/blog/Python-Default-Argument-Mutable-Object/.
(https://github.com/bitcoin/bitcoin/pull/32835#discussion_r2181163397)
that was actually done on purpose. See https://leimao.github.io/blog/Python-Default-Argument-Mutable-Object/.
💬 furszy commented on pull request "test: fix feature_init.py intermittencies":
(https://github.com/bitcoin/bitcoin/pull/32835#discussion_r2181167049)
ups, reverting. Thanks.
(https://github.com/bitcoin/bitcoin/pull/32835#discussion_r2181167049)
ups, reverting. Thanks.
💬 furszy commented on pull request "test: fix feature_init.py intermittencies":
(https://github.com/bitcoin/bitcoin/pull/32835#discussion_r2181173483)
Thanks, but I'm about a ~0 here. I don't really find that change better. I'll just leave the current code unless someone feels strongly about changing it
(https://github.com/bitcoin/bitcoin/pull/32835#discussion_r2181173483)
Thanks, but I'm about a ~0 here. I don't really find that change better. I'll just leave the current code unless someone feels strongly about changing it
💬 furszy commented on pull request "test: fix feature_init.py intermittencies":
(https://github.com/bitcoin/bitcoin/pull/32835#discussion_r2181177845)
This is actually just a leftover from debugging the issue in #26966.. sorry for that. Will just remove the line.
(https://github.com/bitcoin/bitcoin/pull/32835#discussion_r2181177845)
This is actually just a leftover from debugging the issue in #26966.. sorry for that. Will just remove the line.
💬 furszy commented on pull request "test: fix feature_init.py intermittencies":
(https://github.com/bitcoin/bitcoin/pull/32835#discussion_r2181170410)
> It might be good to send in the same startup_args to check_clean_start() further down in the loop?
I don't think so. All files are restored from the backup after deletion, so we know for sure the indexes are synced at that point (nothing changed).
(https://github.com/bitcoin/bitcoin/pull/32835#discussion_r2181170410)
> It might be good to send in the same startup_args to check_clean_start() further down in the loop?
I don't think so. All files are restored from the backup after deletion, so we know for sure the indexes are synced at that point (nothing changed).
💬 furszy commented on pull request "test: fix feature_init.py intermittencies":
(https://github.com/bitcoin/bitcoin/pull/32835#issuecomment-3029743647)
Updated per feedback. Thanks @hodlinator
(https://github.com/bitcoin/bitcoin/pull/32835#issuecomment-3029743647)
Updated per feedback. Thanks @hodlinator
💬 furszy commented on pull request "wallet: Fix relative path backup during migration.":
(https://github.com/bitcoin/bitcoin/pull/32273#discussion_r2181204274)
We are already doing the backup file existence check at the end of `migrate_and_get_rpc`, what if you include this extra check there too?
(https://github.com/bitcoin/bitcoin/pull/32273#discussion_r2181204274)
We are already doing the backup file existence check at the end of `migrate_and_get_rpc`, what if you include this extra check there too?
💬 w0xlt commented on pull request "wallet: Track no-longer-spendable TXOs separately":
(https://github.com/bitcoin/bitcoin/pull/27865#discussion_r2181221787)
Why is it optional instead of false by default?
(https://github.com/bitcoin/bitcoin/pull/27865#discussion_r2181221787)
Why is it optional instead of false by default?
💬 pablomartin4btc commented on pull request "wallet, test: best block locator matches scan state follow-ups":
(https://github.com/bitcoin/bitcoin/pull/32580#discussion_r2181674862)
nit: I haven't seen the format I'm proposing here but just to make it more clear if it makes sense...
```suggestion
self.log.info(
"Test that on a duplicate block disconnection event after unclean shutdown:\n"
" - wallet transactions are un-abandoned after temporarily invalidated blocks;\n"
" - wallet doesn't crash."
)
```
(https://github.com/bitcoin/bitcoin/pull/32580#discussion_r2181674862)
nit: I haven't seen the format I'm proposing here but just to make it more clear if it makes sense...
```suggestion
self.log.info(
"Test that on a duplicate block disconnection event after unclean shutdown:\n"
" - wallet transactions are un-abandoned after temporarily invalidated blocks;\n"
" - wallet doesn't crash."
)
```
🤔 pablomartin4btc reviewed a pull request: "wallet, test: best block locator matches scan state follow-ups"
(https://github.com/bitcoin/bitcoin/pull/32580#pullrequestreview-2981629314)
cr-ACK 1b5c545e82fe3cf5027f16b43e2306aeb8d4ef9b
Reviewed #30221 to verify the agreed follow-ups:
- In `wallet.cpp`:
- another opportunity to use `setLastBlockProcessedInMem` - https://github.com/bitcoin/bitcoin/pull/30221#discussion_r2090917963
- `(!loc.IsNull())` - https://github.com/bitcoin/bitcoin/pull/30221#discussion_r2096279905
- In `wallet_reorgsrestore.py`:
- update test log - https://github.com/bitcoin/bitcoin/pull/30221#discussion_r2091417700
- assert rescanning - http
...
(https://github.com/bitcoin/bitcoin/pull/32580#pullrequestreview-2981629314)
cr-ACK 1b5c545e82fe3cf5027f16b43e2306aeb8d4ef9b
Reviewed #30221 to verify the agreed follow-ups:
- In `wallet.cpp`:
- another opportunity to use `setLastBlockProcessedInMem` - https://github.com/bitcoin/bitcoin/pull/30221#discussion_r2090917963
- `(!loc.IsNull())` - https://github.com/bitcoin/bitcoin/pull/30221#discussion_r2096279905
- In `wallet_reorgsrestore.py`:
- update test log - https://github.com/bitcoin/bitcoin/pull/30221#discussion_r2091417700
- assert rescanning - http
...
👍 rkrux approved a pull request: "wallet: Remove ISMINE_WATCHONLY and watchonly from RPCs"
(https://github.com/bitcoin/bitcoin/pull/32618#pullrequestreview-2981864736)
ACK b1a8ac07e91dd1d305fcbc16ea931d60e46c0055
(https://github.com/bitcoin/bitcoin/pull/32618#pullrequestreview-2981864736)
ACK b1a8ac07e91dd1d305fcbc16ea931d60e46c0055
💬 rkrux commented on pull request "wallet: Remove ISMINE_WATCHONLY and watchonly from RPCs":
(https://github.com/bitcoin/bitcoin/pull/32618#discussion_r2181951390)
Got it. Generally how long do we wait to remove a field from the code after marking it deprecated?
(https://github.com/bitcoin/bitcoin/pull/32618#discussion_r2181951390)
Got it. Generally how long do we wait to remove a field from the code after marking it deprecated?
💬 maflcko commented on pull request "fuzz: Make process_message(s) more deterministic":
(https://github.com/bitcoin/bitcoin/pull/32822#issuecomment-3031079858)
> mock time
My bad and nice catch. Added it there as well.
(https://github.com/bitcoin/bitcoin/pull/32822#issuecomment-3031079858)
> mock time
My bad and nice catch. Added it there as well.
💬 Sjors commented on pull request "cmake: Move internal binaries from bin/ to libexec/":
(https://github.com/bitcoin/bitcoin/pull/31679#discussion_r2182003768)
04e51ba1110e13598e0b11a35ce5abf4a1789f53: now that the wrapper binary is available, the release note could enumerate how to call these now: `bitcoin test`, etc.
(https://github.com/bitcoin/bitcoin/pull/31679#discussion_r2182003768)
04e51ba1110e13598e0b11a35ce5abf4a1789f53: now that the wrapper binary is available, the release note could enumerate how to call these now: `bitcoin test`, etc.
👍 Sjors approved a pull request: "cmake: Move internal binaries from bin/ to libexec/"
(https://github.com/bitcoin/bitcoin/pull/31679#pullrequestreview-2981960982)
re-ACK 705791cd436f237fe9bbac2cf52d63ab4b2a41c7
Left a suggestion for the release note.
(https://github.com/bitcoin/bitcoin/pull/31679#pullrequestreview-2981960982)
re-ACK 705791cd436f237fe9bbac2cf52d63ab4b2a41c7
Left a suggestion for the release note.
💬 maflcko commented on pull request "Fix tiebreak when loading blocks from disk (and add tests for comparing chain ties)":
(https://github.com/bitcoin/bitcoin/pull/29640#discussion_r2182072018)
Possible typos and grammar issues:
In feature_chain_tiebreaks.py: “Make sure than only the former connects” -> “Make sure that only the former connects” [‘than’ should be ‘that’]
In feature_chain_tiebreaks.py: “# Restart and check enough times to this to eventually fail if the logic is broken” -> “# Restart and check this enough times to eventually fail if the logic is broken” [‘to this’ is misplaced and hinders comprehension]
(https://github.com/bitcoin/bitcoin/pull/29640#discussion_r2182072018)
Possible typos and grammar issues:
In feature_chain_tiebreaks.py: “Make sure than only the former connects” -> “Make sure that only the former connects” [‘than’ should be ‘that’]
In feature_chain_tiebreaks.py: “# Restart and check enough times to this to eventually fail if the logic is broken” -> “# Restart and check this enough times to eventually fail if the logic is broken” [‘to this’ is misplaced and hinders comprehension]
💬 maflcko commented on pull request "checkqueue: set MAX_SCRIPTCHECK_THREADS to nCores - 1":
(https://github.com/bitcoin/bitcoin/pull/32692#issuecomment-3031197708)
Maybe turn into draft while CI is red and this is still a WIP?
(https://github.com/bitcoin/bitcoin/pull/32692#issuecomment-3031197708)
Maybe turn into draft while CI is red and this is still a WIP?
💬 maflcko commented on pull request "fee estimate test: fix #31944 by handling a legitimate scenario that …":
(https://github.com/bitcoin/bitcoin/pull/32615#issuecomment-3031268246)
Just ensure enough random transactions have been created to reliably return a fee estimate in any run?
(https://github.com/bitcoin/bitcoin/pull/32615#issuecomment-3031268246)
Just ensure enough random transactions have been created to reliably return a fee estimate in any run?
💬 maflcko commented on issue "fuzz: Fix stability, determinism issues":
(https://github.com/bitcoin/bitcoin/issues/29018#issuecomment-3031280019)
Just dropping an extremely hacky/ugly patch that could be useful to collect the combined debug log while iterating over all fuzz input files in one process and then split that debug log again for each fuzz input file and show the diff:
<details><summary>hacky patch</summary>
```diff
diff --git a/a.py b/a.py
new file mode 100644
index 0000000000..a28b602891
--- /dev/null
+++ b/a.py
@@ -0,0 +1,27 @@
+import os
+import sys
+
+in_a=sys.argv[1]
+out_a=[]
+for basename_a in [in_a,in_a.replace('.a.',
...
(https://github.com/bitcoin/bitcoin/issues/29018#issuecomment-3031280019)
Just dropping an extremely hacky/ugly patch that could be useful to collect the combined debug log while iterating over all fuzz input files in one process and then split that debug log again for each fuzz input file and show the diff:
<details><summary>hacky patch</summary>
```diff
diff --git a/a.py b/a.py
new file mode 100644
index 0000000000..a28b602891
--- /dev/null
+++ b/a.py
@@ -0,0 +1,27 @@
+import os
+import sys
+
+in_a=sys.argv[1]
+out_a=[]
+for basename_a in [in_a,in_a.replace('.a.',
...