💬 fanquake commented on pull request "[WIP] guix: use `-muse-unaligned-vector-move` for Windows builds":
(https://github.com/bitcoin/bitcoin/pull/28151#issuecomment-1650157656)
cc @theuni

📝 instagibbs opened a pull request: "Add benchmark for miniminer"
(https://github.com/bitcoin/bitcoin/pull/28152)
Directly ripped off from the fuzztest

Ideally used to inform design decisions for package relay work: https://github.com/bitcoin/bitcoin/pull/26711#issuecomment-1647523520

👍 vasild approved a pull request: "test: fix intermittent failure in p2p_getaddr_caching.py"
(https://github.com/bitcoin/bitcoin/pull/28144#pullrequestreview-1545936696)
ACK 8a20f765cce2fc0fadf1a2b66b843b67f2d2ae12

💬 instagibbs commented on pull request "validate package transactions with their in-package ancestor sets":
(https://github.com/bitcoin/bitcoin/pull/26711#issuecomment-1650184694)
new approach seems in line with what we've discussed previously. Basically https://github.com/bitcoin/bitcoin/pull/26711#discussion_r1231092018 AJ's suggestion, minus retries, plus using explicit linearization step.

Obvious point but including miniminer dependency means we'll likely have increased review surface, even if it ends up making a lot of sense in this case. From my basic experimentation in https://github.com/bitcoin/bitcoin/pull/28152 it looks like the performance hit is negligible,
...

🤔 glozow reviewed a pull request: "net processing: clamp PeerManager::Options user input"
(https://github.com/bitcoin/bitcoin/pull/28149#pullrequestreview-1545932253)
utACK 128ad03792cd4aeeaf32807d07f01e3f85adaf28

Thanks for the followup

💬 glozow commented on pull request "net processing: clamp PeerManager::Options user input":
(https://github.com/bitcoin/bitcoin/pull/28149#discussion_r1273826506)
nit, includes a bit more than replaced and orphans
```suggestion
//! Number of non-mempool transactions to keep around for block reconstruction. Includes orphan, replaced, and rejected transactions.
```

💬 ismaelsadeeq commented on pull request "Mempool: persist mempoolminfee accross restarts":
(https://github.com/bitcoin/bitcoin/pull/27859#discussion_r1273836288)
@glozow fixed now using `CAmount`
Thank you

🤔 MarcoFalke reviewed a pull request: "wallet: bugfix, disallow migration of invalid scripts"
(https://github.com/bitcoin/bitcoin/pull/28125#pullrequestreview-1545962463)
Concept ACK

💬 MarcoFalke commented on pull request "wallet: bugfix, disallow migration of invalid scripts":
(https://github.com/bitcoin/bitcoin/pull/28125#discussion_r1273846396)
```suggestion
wallet.rpc.importaddress(address=script_sh_pkh.hex(), label="boom_script", rescan=False, p2sh=True)
```

nit: Could skip rescan, since there are no matches?

👍 theuni approved a pull request: "kernel: Remove UniValue from kernel library"
(https://github.com/bitcoin/bitcoin/pull/28113#pullrequestreview-1545977927)
Re-ACK 6960c81cbfa6208d4098353e53b313e13a21cb49

👍 MarcoFalke approved a pull request: "test: fix `feature_addrman.py` on big-endian systems"
(https://github.com/bitcoin/bitcoin/pull/27529#pullrequestreview-1545981390)
review lgtm. Will test later with https://github.com/bitcoin/bitcoin/pull/28087

lgtm ACK 53c990ad3406ee945305af84af98d2f020e5f316 🔚

<details><summary>Show signature</summary>

Signature:

```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N9
...

💬 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;
```