💬 pinheadmz commented on pull request "test: Add unit & functional test coverage for blockstore":
(https://github.com/bitcoin/bitcoin/pull/27850#discussion_r1275371316)
looks good,
runs usdt test: https://cirrus-ci.com/task/4683666694078464?logs=ci#L4600-L4604

📝 brunoerg opened a pull request: "test: remove unused code in `wallet_fundrawtransaction`"
(https://github.com/bitcoin/bitcoin/pull/28164)
This PR removes in `wallet_fundrawtransaction`:
- unecessary variables/calls to `decoderawtransaction`
- unused `totalOut` variable and its related code (`totalOut` is used in some functions to test change, in other ones its value is not used)

💬 jonatack commented on pull request "Relay own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/27509#discussion_r1275391537)
Yes, these three are CJDNS.

💬 pinheadmz commented on pull request "Relay own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/27509#discussion_r1275392099)
does the test node need a config flag to route?

💬 kevkevinpal commented on pull request "test: remove unused code in `wallet_fundrawtransaction`":
(https://github.com/bitcoin/bitcoin/pull/28164#issuecomment-1652416657)
utACK [108c625](https://github.com/bitcoin/bitcoin/pull/28164/commits/108c6255bc670bbf2732f2b79f6c32c26e181208)

I'm fine with just removing the unused variables but instead of removing the `totalOut` and `dec_tx` would it make sense to add an assertion on what that value is? Not exactly sure why the unused vars were added initially

📝 sipa opened a pull request: "net: transport abstraction"
(https://github.com/bitcoin/bitcoin/pull/28165)
This PR furthers the P2P message serialization/deserialization abstraction introduced in #16202 and #16562, in preparation for introducing the BIP324 v2 transport (making this part of #27634). However, nothing in this PR is BIP324-specific, and it contains a number of independently useful improvement.

The overall idea is to have a single object in every `CNode` (called `m_transport`) that is responsible for converting sent messages to wire bytes, and for converting received wire bytes back to
...

💬 achow101 commented on pull request "Rework validation logic for assumeutxo":
(https://github.com/bitcoin/bitcoin/pull/27746#issuecomment-1652419820)
ACK a733dd79e29068ad1e0532ac42a45188a040a7b9

Changes since last are mainly responding to review comments.

👍 jamesob approved a pull request: "BIP324 ciphersuite"
(https://github.com/bitcoin/bitcoin/pull/28008#pullrequestreview-1548082611)
ACK 180909f2c859be3c79630d9705c26a457715b9ed ([`jamesob/ackr/28008.3.sipa.bip324_ciphersuite`](https://github.com/jamesob/bitcoin/tree/ackr/28008.3.sipa.bip324_ciphersuite))

Looks great. I've spent about a week ramping up on BIP-324, reading the BIPs and underlying RFC 8439, asking @sipa stupid questions about the garbage terminator, and carefully reviewing the code in this pull request. I've verified the ChaCha20-Poly1305 test vectors against an unrelated implementation (Python's chacha20pol
...

💬 jamesob commented on pull request "BIP324 ciphersuite":
(https://github.com/bitcoin/bitcoin/pull/28008#discussion_r1275173737)
9e65744c4f44ce89f3176870d3cc25ac2b6bdc08

Only if you retouch, might be nice to declare the values that don't change here as `const auto`.

💬 jamesob commented on pull request "BIP324 ciphersuite":
(https://github.com/bitcoin/bitcoin/pull/28008#discussion_r1275238979)
9e65744c4f44ce89f3176870d3cc25ac2b6bdc08

Slightly confused by why this conditional is necessary - can you explain in what case our expected ciphertext would be empty, but the actually generated ciphertext wouldn't be? I've tried removing this conditional but I get test failures.

💬 jamesob commented on pull request "BIP324 ciphersuite":
(https://github.com/bitcoin/bitcoin/pull/28008#discussion_r1275295605)
9e65744c4f44ce89f3176870d3cc25ac2b6bdc08

Small thing, but if you retouch maybe:

```diff
diff --git a/src/test/fuzz/bip324.cpp b/src/test/fuzz/bip324.cpp
index 5df279b7df..376728d61d 100644
--- a/src/test/fuzz/bip324.cpp
+++ b/src/test/fuzz/bip324.cpp
@@ -109,8 +109,10 @@ FUZZ_TARGET(bip324_cipher_roundtrip, .init=Initialize)
}
}

+ auto& receiver{from_init ? responder : initiator};
+
// Decrypt length
- uint32_t dec_length = (from_in
...

💬 jamesob commented on pull request "BIP324 ciphersuite":
(https://github.com/bitcoin/bitcoin/pull/28008#discussion_r1275292903)
9e65744c4f44ce89f3176870d3cc25ac2b6bdc08

Choice of 2**14 arbitrary?

💬 achow101 commented on pull request "Silent Payments: Implement BIP352":
(https://github.com/bitcoin/bitcoin/pull/28122#discussion_r1275442608)
In 56882622faf469b6f948f79a69c3c8ddbde92ff8 "crypto: update CKey, CPubkey for silent payments"

Same data copying concerns as `AddTweak`.

💬 achow101 commented on pull request "Silent Payments: Implement BIP352":
(https://github.com/bitcoin/bitcoin/pull/28122#discussion_r1275442463)
In 56882622faf469b6f948f79a69c3c8ddbde92ff8 "crypto: update CKey, CPubkey for silent payments"

We shouldn't be copying secret data out of a secure structure into an insecure one. When `tweaked_seckey` is destroyed, the underlying data may still linger in memory and end up being read by something else.

If this function must return a new `CKey`, then it should instantiate it here and perform the operation on it's `data()` rather than doing this copy.

However, I would prefer if this functi
...

💬 pinheadmz commented on pull request "p2p: Allow whitelisting outgoing connections":
(https://github.com/bitcoin/bitcoin/pull/27114#discussion_r1275426912)
yeah? 😏

🤔 pinheadmz reviewed a pull request: "p2p: Allow whitelisting outgoing connections"
(https://github.com/bitcoin/bitcoin/pull/27114#pullrequestreview-1548474448)
concept ACK

Ran all tests locally. Built and reviewed code at each commit. Some questions/comments below. Also I notice this is a fairly old feature request that has been through a few PRs. I wonder if you could address Greg's comments on [this older version](https://github.com/bitcoin/bitcoin/pull/10051)

💬 pinheadmz commented on pull request "p2p: Allow whitelisting outgoing connections":
(https://github.com/bitcoin/bitcoin/pull/27114#discussion_r1275411963)
Also I think it's a bit unclear where the "additional flags" go, are they part of `[permissions@]` ?

💬 pinheadmz commented on pull request "p2p: Allow whitelisting outgoing connections":
(https://github.com/bitcoin/bitcoin/pull/27114#discussion_r1275409948)
Seems weird to use a string placeholder here for `"incoming only"` when there isn't a variable

💬 pinheadmz commented on pull request "p2p: Allow whitelisting outgoing connections":
(https://github.com/bitcoin/bitcoin/pull/27114#discussion_r1275440729)
I'm not sure this is sufficient functional test coverage. With the exception of the new permissions "in" and "out" which are invalid on master, all tests otherwise pass with this commit (1e09c265a9598df3cc39336e3bcccb993dacf3d8) applied.
I was expecting to see a test that makes an outbound connection to a node that misbehaves and doesn't get banned.

Or was the motivation more about the tx "trickle"? In that case I can see it being hard to test without waiting for time to pass?

👍 Ayush170-Future approved a pull request: "fuzz: add target for `ScriptPubKeyMan` (legacy)"
(https://github.com/bitcoin/bitcoin/pull/28153#pullrequestreview-1548543088)
ACK

I support the idea of having separate files for Legacy and Descriptor because I think the Wallet implementation would differ. Overall, the code looks great to me.

💬 sipa commented on pull request "BIP324 ciphersuite":
(https://github.com/bitcoin/bitcoin/pull/28008#discussion_r1275488694)
Added a comment to explain.