💬 rkrux commented on pull request "wallet, rpc: add UTXO set check and incremental rescan to importdescriptors":
(https://github.com/bitcoin/bitcoin/pull/33392#discussion_r2514222126)
In https://github.com/bitcoin/bitcoin/commit/82a222b00d6497dc22e85205dae577a1adf15989 "rpc: extend importdescriptors with UTXO check and incremental rescan"

Shouldn't the timestamp of the imported descriptors be updated as well in this case to avoid data inconsistency?

💬 rkrux commented on pull request "wallet, rpc: add UTXO set check and incremental rescan to importdescriptors":
(https://github.com/bitcoin/bitcoin/pull/33392#discussion_r2514186801)
In fa3ac7378ca016c598d51fa00a9cc02103754569 "wallet/rpc: add scan_utxoset arg & docs for importdescriptors"

```
s/"scan_utxoset"/"verify"
```

From a user's POV, this feels to me more like a verification step that the wallet might do if set by the user. Let's just call it that to keep it simple for the user?

💬 rkrux commented on pull request "wallet, rpc: add UTXO set check and incremental rescan to importdescriptors":
(https://github.com/bitcoin/bitcoin/pull/33392#discussion_r2514139728)
In 82a222b00d6497dc22e85205dae577a1adf15989 "rpc: extend importdescriptors with UTXO check and incremental rescan"

The lack of nesting in the `if` blocks makes the code harder to read.
```diff
diff --git a/src/wallet/rpc/backup.cpp b/src/wallet/rpc/backup.cpp
index 4146a65876..cf8269b2ef 100644
--- a/src/wallet/rpc/backup.cpp
+++ b/src/wallet/rpc/backup.cpp
@@ -507,7 +507,6 @@ RPCHelpMan importdescriptors()
bool have_prune_boundary = false;
int64_t min_trial_start_time = 0;
...

💬 rkrux commented on pull request "wallet, rpc: add UTXO set check and incremental rescan to importdescriptors":
(https://github.com/bitcoin/bitcoin/pull/33392#discussion_r2514188570)
In 1ed236e3c686582e35f20ab668cd6d93316fae2c "test: add functional test for importdescriptors scan_utxo flag"

What's the reason to assert this at only this stage?

💬 rkrux commented on pull request "wallet, rpc: add UTXO set check and incremental rescan to importdescriptors":
(https://github.com/bitcoin/bitcoin/pull/33392#discussion_r2514333367)
In https://github.com/bitcoin/bitcoin/commit/fa3ac7378ca016c598d51fa00a9cc02103754569 "wallet/rpc: add scan_utxoset arg & docs for importdescriptors"

I think these don't need to be marked optional when the `info` object is already marked so. Unless these properties appear optionally inside the `info` object that I think is not the case.

💬 rkrux commented on pull request "wallet, rpc: add UTXO set check and incremental rescan to importdescriptors":
(https://github.com/bitcoin/bitcoin/pull/33392#discussion_r2514350932)
In ac06ddb8b9e33b7a594af9e38e00f2a8410947b7 "wallet: add GetWalletUTXOSetBalance to calculate balance from UTXO set"

I don't think the bifurcation between `output_scripts_mine` and `output_scripts_watchonly` needs to be done anymore because the watch only property in the descriptor wallets is at the wallet level now. Either the whole wallet will be watch-only or all of it will be not. Ref: #32618

💬 rkrux commented on pull request "wallet, rpc: add UTXO set check and incremental rescan to importdescriptors":
(https://github.com/bitcoin/bitcoin/pull/33392#discussion_r2514313528)
In https://github.com/bitcoin/bitcoin/commit/fa3ac7378ca016c598d51fa00a9cc02103754569 "wallet/rpc: add scan_utxoset arg & docs for importdescriptors"

Why is this comment cut midway?

💬 ryanofsky commented on pull request "kernel: Improve logging API":
(https://github.com/bitcoin/bitcoin/pull/33847#issuecomment-3517178327)
<!-- begin push-2 -->
Updated d0bd114e974500ad11c8ecac26d6606166ab7438 -> 0926479924f453c38b5e0ad069435fb3241b2389 ([`pr/klog.1`](https://github.com/ryanofsky/bitcoin/commits/pr/klog.1) -> [`pr/klog.2`](https://github.com/ryanofsky/bitcoin/commits/pr/klog.2), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/klog.1..pr/klog.2))<!-- end --> updating btck_logging_connection_create documentation

💬 ismaelsadeeq commented on pull request "mining: check witness commitment in submitBlock":
(https://github.com/bitcoin/bitcoin/pull/33745#issuecomment-3517184939)
See https://github.com/bitcoin/bitcoin/pull/33421#discussion_r2514393534

That PR should be updated. This is the right direction IMO.

💬 optout21 commented on pull request "refactor: C++20 operators":
(https://github.com/bitcoin/bitcoin/pull/33771#issuecomment-3517219391)
utACK 48840bfc2d7beeac0ddf56a3c26b243156ec8936

Straightforward refactoring resulting in code removal 😎
First commit removes `!=` operator where `==` exits. Second commit replaces 4 comparison operators with the new `<=>` spaceship operator.
My only concern is that competence with the new `<=>` operator may not be extensive across the developer community of this project, but hey, it's time to learn!

✅ maflcko closed a pull request: "ci: Enable experimental kernel stuff in ASan task"
(https://github.com/bitcoin/bitcoin/pull/33845)

💬 maflcko commented on pull request "ci: Enable experimental kernel stuff in ASan task":
(https://github.com/bitcoin/bitcoin/pull/33845#issuecomment-3517219870)
Thanks for picking this up! Looks like a simple and small change in your pull request.

💬 optout21 commented on pull request "refactor: C++20 operators":
(https://github.com/bitcoin/bitcoin/pull/33771#issuecomment-3517222722)
I wonder whether it would it be possible check for superfluous comparison operator implementations in linting?

💬 maflcko commented on pull request "kernel: Allow null arguments for serialized data":
(https://github.com/bitcoin/bitcoin/pull/33853#discussion_r2514464091)
```suggestion
BOOST_CHECK_THROW(Transaction{std::span{static_cast<std::byte*>(nullptr), 2}}, std::runtime_error);
```

style-nit in 5b89956eeb76cf8c9717152fbb0928e026fc0087 (no need to re-touch), also I haven't tried compiling, but I think the compiler can derive the type here and it can be omitted. Same below.

👍 maflcko approved a pull request: "kernel: Allow null arguments for serialized data"
(https://github.com/bitcoin/bitcoin/pull/33853#pullrequestreview-3448360329)
Make sense to handle empty spans at runtime properly via our serialization and the existing serialize-error-handling, to avoid runtime violations of the non-null attribute.

review ACK a3ac59a4316305fb38a5338b48940682889d0dc2 🥈

<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_fou
...

🤔 mzumsande reviewed a pull request: "doc: document fingerprinting risk when operating node on multiple networks"
(https://github.com/bitcoin/bitcoin/pull/33750#pullrequestreview-3448401470)
ACK https://github.com/bitcoin/bitcoin/commit/e346ecae830e10310979e5f64de63e043a383ff1

nit: Commit separation is still not clean (second commit still touches tor.md).

✅ fanquake closed an issue: "Fuzz: compare our AES implementation to AES-NI "
(https://github.com/bitcoin/bitcoin/issues/27548)

💬 fanquake commented on issue "Fuzz: compare our AES implementation to AES-NI ":
(https://github.com/bitcoin/bitcoin/issues/27548#issuecomment-3517243078)
Will close for now, and see what comes out of https://github.com/bitcoinfuzz/bitcoinfuzz/issues/321.

💬 mzumsande commented on pull request "p2p: Advance pindexLastCommonBlock early after connecting blocks":
(https://github.com/bitcoin/bitcoin/pull/32180#discussion_r2514514693)
Sorry, I missed this. yes, it's a bit ambiguous, the intention is "required for `FindNestBlocks`" (it's also used as a starting point there). This could be made more precise in a follow-up.

👋 fanquake's pull request is ready for review: "[29.x] Backports"
(https://github.com/bitcoin/bitcoin/pull/33611)