💬 sipa commented on pull request "BIP324 ciphersuite":
(https://github.com/bitcoin/bitcoin/pull/28008#discussion_r1273858928)
See #28100, which modernizes `class ChaCha20` too.

💬 theuni commented on pull request "kernel: Remove UniValue from kernel library":
(https://github.com/bitcoin/bitcoin/pull/28113#issuecomment-1650227465)
I think the header split was reasonable btw, and I think it's safe to assume it may be redone in the future, but not for the stated reasons. Breaking something like that out for the sake of modularity and reducing dependencies is fine, but I agree with @fanquake that even if it did speed up compile, that alone would not be a compelling enough reason to reorganize the code.

💬 brunoerg commented on pull request "Silent Payments: send and receive":
(https://github.com/bitcoin/bitcoin/pull/27827#discussion_r1262821441)
In 96f7a3a0c0ccf81269a587dd2aa52aaf895e0046: you could simplify it by doing:
```diff
diff --git a/test/functional/wallet_silentpayment_blockfilterindex.py b/test/functional/wallet_silentpayment_blockfilterindex.py
index 16d467c18..8f5127ae1 100755
--- a/test/functional/wallet_silentpayment_blockfilterindex.py
+++ b/test/functional/wallet_silentpayment_blockfilterindex.py
@@ -90,10 +90,8 @@ class SilentIdentifierSimple(BitcoinTestFramework):
self.nodes[0].createwallet(wallet_name=
...

💬 brunoerg commented on pull request "Silent Payments: send and receive":
(https://github.com/bitcoin/bitcoin/pull/27827#discussion_r1262819269)
In 96f7a3a0c0ccf81269a587dd2aa52aaf895e0046: if `i` has not been used, so it could be `for _ in range(50)`

💬 brunoerg commented on pull request "Silent Payments: send and receive":
(https://github.com/bitcoin/bitcoin/pull/27827#discussion_r1262830107)
In `c4d64fadae0d77da22e0eec91a97c8f3290ff8c3`: You could use `silent_payment_hrp` instead of calling `params.SilentPaymentHRP()` again.

```diff
diff --git a/src/key_io.cpp b/src/key_io.cpp
index c6ad8dc1a..105772aab 100644
--- a/src/key_io.cpp
+++ b/src/key_io.cpp
@@ -347,7 +347,7 @@ std::vector<unsigned char> DecodeSilentAddress(const std::string& str)
const auto& params{Params()};

const auto& silent_payment_hrp = params.SilentPaymentHRP();
- auto dest_silent_payment
...

💬 brunoerg commented on pull request "Silent Payments: send and receive":
(https://github.com/bitcoin/bitcoin/pull/27827#discussion_r1262845441)
in ccb4c1d23db7bf33f2914270d354f0e5a2f93a48: It seems like `block_hash` has not been used in `GetSilentPaymentKeysPerBlock`.

💬 brunoerg commented on pull request "Silent Payments: send and receive":
(https://github.com/bitcoin/bitcoin/pull/27827#discussion_r1262864274)
In c4d64fadae0d77da22e0eec91a97c8f3290ff8c3: `dec.data[0]` may cause a segmentation fault.

You are trying to access `data[0]` right before checking if `data` is empty.
```cpp
auto version = dec.data[0];
if (dec.encoding != bech32::Encoding::BECH32M || dec.data.empty() || dec.hrp != silent_payment_hrp || version != 0) {
return {};
}
```

Note, a simple fuzzer would catch it, e.g.:
```diff
diff --git a/src/test/fuzz/key_io.cpp b/src/test/fuzz/key_io.cpp
index a1c587a75..9b5a4b278
...

💬 brunoerg commented on pull request "Silent Payments: send and receive":
(https://github.com/bitcoin/bitcoin/pull/27827#discussion_r1262833777)
In c4d64fadae0d77da22e0eec91a97c8f3290ff8c3: This `clear` call is unnecessary here.

💬 brunoerg commented on pull request "Silent Payments: send and receive":
(https://github.com/bitcoin/bitcoin/pull/27827#discussion_r1262873786)
In 3a0d081ee07f3c65220a45855c96bf35ac2643fd: `pwallet` seems unecessary here? perhaps it's a leftover?

```cpp
UniValue ret(UniValue::VOBJ);
ret.pushKV("address", request.params[0].get_str());
ret.pushKV("scan_pubkey", HexStr(scan_pubkey));
ret.pushKV("spend_pubkey", HexStr(spend_pubkey));

const std::shared_ptr<const CWallet> pwallet = GetWalletForJSONRPCRequest(request);
if (!pwallet) return ret;

LOCK(pwallet->cs_wallet);

return ret;
```

💬 brunoerg commented on pull request "Silent Payments: send and receive":
(https://github.com/bitcoin/bitcoin/pull/27827#discussion_r1263749891)
In 12731a823d9ed3fde5dd19521da5d7544fcb07dd: Perhaps we could add a check/assert to ensure that `solutions` is not empty?

💬 brunoerg commented on pull request "Silent Payments: send and receive":
(https://github.com/bitcoin/bitcoin/pull/27827#discussion_r1262887619)
In 3a0d081ee07f3c65220a45855c96bf35ac2643fd: s/paymet/payment

👍 stickies-v approved a pull request: "kernel: Remove UniValue from kernel library"
(https://github.com/bitcoin/bitcoin/pull/28113#pullrequestreview-1546015873)
re-ACK https://github.com/bitcoin/bitcoin/commit/6960c81cbfa6208d4098353e53b313e13a21cb49

💬 MarcoFalke commented on pull request "test: Avoid intermittent issues due to async events in validationinterface_tests":
(https://github.com/bitcoin/bitcoin/pull/28150#issuecomment-1650265913)
> Using ChainTestingSetup should be faster?

It is doing less setup, so it may also be faster.

💬 brunoerg commented on pull request "Silent Payments: send and receive":
(https://github.com/bitcoin/bitcoin/pull/27827#discussion_r1273901955)
```suggestion
{RPCResult::Type::BOOL, "silent_payment", "whether this wallet supports silent payments"},
```

💬 brunoerg commented on pull request "Silent Payments: send and receive":
(https://github.com/bitcoin/bitcoin/pull/27827#discussion_r1273902470)
```suggestion
{"silent_payment", RPCArg::Type::BOOL, RPCArg::Default{false}, "Experimental. Indicates that the wallet supports Silent Payments. This means a more complex scanning logic."},
```

👍 john-moffett approved a pull request: "util: Don't derive secure_allocator from std::allocator"
(https://github.com/bitcoin/bitcoin/pull/27930#pullrequestreview-1546065248)
ACK 07c59eda00841aafaafd8fd648217b56b1e907c9 Reviewed and tested. Performance appears unaffected in my environment.

💬 luke-jr commented on pull request "util: Type-safe transaction identifiers":
(https://github.com/bitcoin/bitcoin/pull/28107#issuecomment-1650329917)
Concept ACK, but I'm not too sure about the `Wtxid` type. It's literally just a hash of the transaction, not really an identifier per se.

💬 luke-jr commented on pull request "p2p: make block download logic aware of limited peers threshold":
(https://github.com/bitcoin/bitcoin/pull/28120#issuecomment-1650338677)
Agree with @mzumsande and @naumenkogs. Maybe not the same PR if it's substantially different, but the new eviction logic should be merged before this fix.

💬 luke-jr commented on pull request "include verbose debug messages in testmempoolaccept reject-reason":
(https://github.com/bitcoin/bitcoin/pull/28121#issuecomment-1650342544)
Why not pass the details in `m_debug_message` and add that as a new field?

💬 luke-jr commented on pull request "Bugfix: RPC: Remove quotes from non-string oneline descriptions":
(https://github.com/bitcoin/bitcoin/pull/28123#discussion_r1273943679)
I don't think there is any currently-supported case where `template_request` can be omitted. For a new template, the client must indicate it supports segwit with the rules key. For a proposal, there must be data.