🤔 rkrux reviewed a pull request: "wallet, rpc: add UTXO set check and incremental rescan to importdescriptors"
(https://github.com/bitcoin/bitcoin/pull/33392#pullrequestreview-3447945798)
Concept ACK and cursory review 1ed236e3c686582e35f20ab668cd6d93316fae2c.

The intent of the PR and the corresponding issue #28898 seems fine to me.

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

Why is the UTXO set balance check (and the corresponding incremental block scan incase of balance mismatch) done before the usual rescan that's supposed to be done upon a successful descriptor import? Should this not be done after the usual rescan to find any transactions in the older blocks that might have been missed in the rescan becau
...

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

This string property seems unnecessary if it was have only one value all the time.

💬 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)