💬 instagibbs commented on pull request "validation: docs and cleanups for MemPoolAccept coins views":
(https://github.com/bitcoin/bitcoin/pull/32973#discussion_r2211313016)
s/has now been spent/has now been spent in the mempool/ ?

💬 glozow commented on pull request "wallet, rpc: add v3 transaction creation and wallet support":
(https://github.com/bitcoin/bitcoin/pull/32896#discussion_r2211351922)
maybe these should live in mempool_util?

💬 glozow commented on pull request "wallet, rpc: add v3 transaction creation and wallet support":
(https://github.com/bitcoin/bitcoin/pull/32896#discussion_r2211362713)
Might want to assert that it's v2

💬 glozow commented on pull request "wallet, rpc: add v3 transaction creation and wallet support":
(https://github.com/bitcoin/bitcoin/pull/32896#discussion_r2211355006)
This looks like it was copy-pasted from the test framework - can we just use the existing method, perhaps with a wrapper if needed?

💬 glozow commented on pull request "wallet, rpc: add v3 transaction creation and wallet support":
(https://github.com/bitcoin/bitcoin/pull/32896#discussion_r2211375162)
Could you also add `v2_tx_spends_confirmed_v3_tx` and `v3_tx_spends_confirmed_v2_tx` to check that version mismatches are fine there?

💬 glozow commented on pull request "wallet, rpc: add v3 transaction creation and wallet support":
(https://github.com/bitcoin/bitcoin/pull/32896#discussion_r2211372152)
Should there also be a test where Alice tries to spend her change after Bob has spent from the parent? That wouldn't require `include_unsafe` IIUC

💬 glozow commented on pull request "wallet, rpc: add v3 transaction creation and wallet support":
(https://github.com/bitcoin/bitcoin/pull/32896#discussion_r2211379860)
What does "two outputs" mean here? This has 1 output, no?

💬 glozow commented on pull request "wallet, rpc: add v3 transaction creation and wallet support":
(https://github.com/bitcoin/bitcoin/pull/32896#discussion_r2211390451)
Isn't this just...?
```suggestion
alice_v2_unspent = self.alice.listunspent(minconf=1)[0]
```

💬 glozow commented on pull request "wallet, rpc: add v3 transaction creation and wallet support":
(https://github.com/bitcoin/bitcoin/pull/32896#discussion_r2211381702)
Will Alice's wallet attempt to rebroadcast her transaction if/when the parent + Bob's confirm? Can we do an RBF of Alice's transaction to evict Bob's?

💬 glozow commented on pull request "wallet, rpc: add v3 transaction creation and wallet support":
(https://github.com/bitcoin/bitcoin/pull/32896#discussion_r2211385875)
```suggestion
# send a v2 output to alice and confirm it
````

💬 glozow commented on pull request "wallet, rpc: add v3 transaction creation and wallet support":
(https://github.com/bitcoin/bitcoin/pull/32896#discussion_r2211403206)
Can this be consolidated with `v3_conflict_removed_from_mempool` since they are mostly the same?

🤔 w0xlt reviewed a pull request: "rpc: refactor: use string_view in Arg/MaybeArg"
(https://github.com/bitcoin/bitcoin/pull/32983#pullrequestreview-3026746517)
ACK https://github.com/bitcoin/bitcoin/pull/32983/commits/319abd609b3d2194e7a05454b6af55b6f16adbf1

🤔 l0rinc reviewed a pull request: "[IBD] multi-byte block obfuscation"
(https://github.com/bitcoin/bitcoin/pull/31144#pullrequestreview-3026774129)
Thanks for the hints, the latest version simplifies reading the obfuscation key by using the new `Obfuscation` object's serialization paths + reading back the new key directly after generating a new one.

The `obfuscation_constructors` test was also changes to validate the new `HexKey` conversion which we need since we're logging the generated obfuscations keys so we need to peek into the object.

It also avoids a possible `Obfuscation` copy used in `GetObfuscation` and removed the `obfuscat
...

💬 l0rinc commented on pull request "[IBD] multi-byte block obfuscation":
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2211528848)
Done

💬 l0rinc commented on pull request "[IBD] multi-byte block obfuscation":
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2211529426)
Let me know what you think of the result now.

💬 l0rinc commented on pull request "[IBD] multi-byte block obfuscation":
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2211529189)
> m_obfuscation = Obfuscation{FastRandomContext{}.randbytes<8>()};
Write(OBFUSCATION_KEY_KEY, m_obfuscation);

If we enable obfuscation (by assigning the field) before calling `Write`, it will obfuscate the new key *with* the new key in `CDBBatch::WriteImpl`, so before writing we need `m_obfuscation` to remain empty. I've extended the comments to clarify that.

> FastRandomContext calls GetRandBytes internally

Nice, if we can indeed use `FastRandomContext{}`, the code can be simplified a
...

💬 l0rinc commented on pull request "[IBD] multi-byte block obfuscation":
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2211529040)
Removed from intermediary commit

🤔 ryanofsky reviewed a pull request: "[IBD] multi-byte block obfuscation"
(https://github.com/bitcoin/bitcoin/pull/31144#pullrequestreview-3026270097)
Code review 7b93fa394acaee7a715fae9a2b968d33c5adc174. Still in progress reviewing but wanted to post comments I had so far. There was a new push since these were written so please ignore anything that no longer applies.

💬 ryanofsky commented on pull request "[IBD] multi-byte block obfuscation":
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2211205949)
In commit "test: compare util::Xor with randomized inputs against simple impl" (7750bfe17e1c5cb69806f9f83675cd1e78083f7f)

This seems like a weak check which is not actually verifying the obfuscation is done correctly. Why not just check that obfuscated data is correct?

```diff
--- a/src/test/streams_tests.cpp
+++ b/src/test/streams_tests.cpp
@@ -37,9 +37,10 @@ BOOST_AUTO_TEST_CASE(xor_roundtrip_random_chunks)
const auto key_bytes{m_rng.randbool() ? m_rng.randbytes<Obfuscation:
...

💬 ryanofsky commented on pull request "[IBD] multi-byte block obfuscation":
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2211268495)
In commit "test: compare util::Xor with randomized inputs against simple impl" (7750bfe17e1c5cb69806f9f83675cd1e78083f7f)

There are a lot of changes in this commit which seem to be made without a clear reason and aren't mentioned at all in the commit message.

I understand the point of adding the new `xor_roundtrip_random_chunks` and `xor_bytes_reference` tests but none of the other changes including the rewrite of the main `dbwrapper` test at the top, and the random tweaks such as this see
...