💬 ryanofsky commented on pull request "qa: Verify clean shutdown on startup failure":
(https://github.com/bitcoin/bitcoin/pull/30660#discussion_r2080245499)
In commit "qa: assert_raises_message() - Stop assuming certain structure for exceptions" (28e282ef9ae94ede4aace6b97ff18c66cb72a001)

Would be nice if commit message mentioned why this change is being made. Currently there's no explanation and the change seems pretty random.

Also could replace `{repr(e)}` with just `{e!r}`

💬 ryanofsky commented on pull request "qa: Verify clean shutdown on startup failure":
(https://github.com/bitcoin/bitcoin/pull/30660#discussion_r2080321673)
In commit "qa: Add feature_framework_startup_failures.py" (10845cd7cc89fc83b4ee844dc577b391720bba9c)

Would suggest `s/only modifying tmpdir/just appending a unique suffix to tmpdir/` to give a little more context and help the next part of the comment make sense.

💬 ryanofsky commented on pull request "qa: Verify clean shutdown on startup failure":
(https://github.com/bitcoin/bitcoin/pull/30660#discussion_r2080292165)
In commit "qa: Add feature_framework_startup_failures.py" (10845cd7cc89fc83b4ee844dc577b391720bba9c)

I think all these nested string operations make this code hard to decipher, and flattening could make it more readable. Would suggest:

```diff
--- a/test/functional/feature_framework_startup_failures.py
+++ b/test/functional/feature_framework_startup_failures.py
@@ -71,12 +71,13 @@ class FeatureFrameworkStartupFailures(BitcoinTestFramework):

def run_test(self):
self.lo
...

💬 ryanofsky commented on pull request "qa: Verify clean shutdown on startup failure":
(https://github.com/bitcoin/bitcoin/pull/30660#discussion_r2080314268)
In commit "qa: Add feature_framework_startup_failures.py" (10845cd7cc89fc83b4ee844dc577b391720bba9c)

This line is pretty dense. Might suggest adding a comment like "Get position and value of the first --tmpdir option, or the end position and self.options.tmpdir if none is specified." so it's not necessary to spend time deciphering this to understand the rest of the code.

Also I think it would probably be more correct to `s/enumerate(parent_args)/reversed(enumerate(parent_args))/` to use th
...

💬 ryanofsky commented on pull request "qa: Verify clean shutdown on startup failure":
(https://github.com/bitcoin/bitcoin/pull/30660#discussion_r2080377055)
re: https://github.com/bitcoin/bitcoin/pull/30660#discussion_r2080132673

Suggestion here seems to be to replace latest error `repr` with ignored errors `str`. Could also do the opposite and use `repr` for both. Either way seems find and would be nice to be consistent

💬 ryanofsky commented on pull request "qa: Verify clean shutdown on startup failure":
(https://github.com/bitcoin/bitcoin/pull/30660#discussion_r2080383095)
re: https://github.com/bitcoin/bitcoin/pull/30660#discussion_r2080170811

I didn't look into it closely but I think I don't understand why wait_for_rpc_connection should not be called if stop if called. If there's no cookie file because password authentication is used instead for example, i'd think you should still need for the RPC interface to wait before sending the stop RPC

💬 l0rinc commented on pull request "contrib: add xor-blocks tool to obfuscate blocks directory":
(https://github.com/bitcoin/bitcoin/pull/32451#discussion_r2080400271)
To speed this up significantly, we could probably process the data in 8 byte chunks, cast it to 64 bits, xor it with a 64 bit key and do the last bytes (if any) byte-by-byte against the current vector key - similarly to https://github.com/bitcoin/bitcoin/pull/31144

💬 kevkevinpal commented on pull request "test: added fuzz coverage for consensus/merkle.cpp":
(https://github.com/bitcoin/bitcoin/pull/32243#discussion_r2080404024)
Good catch! You're right it doesnt make sense to always have the `MAX_SCRIPT_SIZE` we should instead use in that range

updated in [0fc30f2](https://github.com/bitcoin/bitcoin/pull/32243/commits/0fc30f215f0f9c630b0c84a6ce2b89c6d4135237)

💬 fjahr commented on pull request "Embed default ASMap as binary dump header file":
(https://github.com/bitcoin/bitcoin/pull/28792#discussion_r2080421333)
Good idea. Done, also made a BE version of it.

💬 fjahr commented on pull request "Embed default ASMap as binary dump header file":
(https://github.com/bitcoin/bitcoin/pull/28792#discussion_r2080421426)
Done

💬 fjahr commented on pull request "Embed default ASMap as binary dump header file":
(https://github.com/bitcoin/bitcoin/pull/28792#discussion_r2080421642)
Hm, right, that's a left-over from previous iterations. Done.

💬 brunoerg commented on pull request "fuzz: wallet: add target for spkm migration":
(https://github.com/bitcoin/bitcoin/pull/29694#issuecomment-2864238675)
I'm reworking this target, will draft it.

📝 brunoerg converted_to_draft a pull request: "fuzz: wallet: add target for spkm migration"
(https://github.com/bitcoin/bitcoin/pull/29694)
This PR adds a fuzz target for `ScriptPubKeyMan` migration. It creates a `LegacyDataSPKM` which can have some keys/scripts/etc, and then migrate it to descriptor. I tried to keep it as compatible as possible with future legacy wallet removal.

💬 fjahr commented on pull request "Embed default ASMap as binary dump header file":
(https://github.com/bitcoin/bitcoin/pull/28792#discussion_r2080421844)
Taken

💬 fjahr commented on pull request "Embed default ASMap as binary dump header file":
(https://github.com/bitcoin/bitcoin/pull/28792#discussion_r2080421917)
Done

💬 fjahr commented on pull request "Embed default ASMap as binary dump header file":
(https://github.com/bitcoin/bitcoin/pull/28792#discussion_r2080422936)
Taken but this code skipped the last byte if the input wasn't a multiple of 8, so I added that.

💬 fjahr commented on pull request "Embed default ASMap as binary dump header file":
(https://github.com/bitcoin/bitcoin/pull/28792#discussion_r2080425401)
Right, I adapted this part to skip the last byte since the `BitsToBytes` function would pad again, but otherwise I think this now functions as the original fuzz test again.

💬 brunoerg commented on pull request "test: added fuzz coverage for consensus/merkle.cpp":
(https://github.com/bitcoin/bitcoin/pull/32243#discussion_r2080425465)
Why not using `ConsumeScript` to create the scriptPubKey and scriptSig?

💬 pinheadmz commented on pull request "tests: Expand HTTP coverage to assert libevent behavior":
(https://github.com/bitcoin/bitcoin/pull/32408#discussion_r2080431221)
I noticed in the libevent's test they don't actually check that server waited at all, just that the connection closed within a few seconds of the configured time out. So I went that route, but also added a tiny minimum check just to make sure the server isn't closing immediately. And this gives us better test reliability.

So the new test is: `-rpcservertimeout=2`, we send a request, we time how long it takes to close, and that duration is expected to be between 1 and 4 seconds.

💬 keith-gardner commented on pull request "Remove arbitrary limits on OP_Return (datacarrier) outputs":
(https://github.com/bitcoin/bitcoin/pull/32359#issuecomment-2864283170)
Concept NACK