💬 maflcko commented on pull request "[IBD] multi-byte block obfuscation":
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2207004439)
nit in 72c775d58a70771efa48acf12da7a6d434adf04e: Could make sense to move this file to `src/util`, to avoid bloating the root source dir. I know this is a stand-alone header right now, but in the future this could also make it easier to move it to the util lib, if there is need.

👍 maflcko approved a pull request: "[IBD] multi-byte block obfuscation"
(https://github.com/bitcoin/bitcoin/pull/31144#pullrequestreview-3019558737)
looked at a6d254e2462cb0d4276e5388b540e02ebc2366c9

💬 maflcko commented on pull request "[IBD] multi-byte block obfuscation":
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2207018700)
nit in 72c775d58a70771efa48acf12da7a6d434adf04e:

I still don't understand why the same line is changed/renamed several times, when it could just be changed a single time.

it is changed xor_key -> obfuscation -> key_bytes


Also, first hard-coding `8` and then changing all the same lines to `Obfuscation::KEY_SIZE` seems odd. Why not let the very first commit be:

```
class Obfuscation
{
public:
static constexpr size_t KEY_SIZE{sizeof(uint64_t)};
};
```

and then use that fr
...

💬 maflcko commented on issue "GUI bitcoin core shows wrong amount":
(https://github.com/bitcoin/bitcoin/issues/32976#issuecomment-3072971242)
You can use the coin selection expert feature, or the RPC interface

💬 fanquake commented on pull request "cmake: Move internal binaries from bin/ to libexec/":
(https://github.com/bitcoin/bitcoin/pull/31679#issuecomment-3073060232)
@theuni @hebasto @purpleKarrot any opinons/discussion you want to add?

✅ fanquake closed an issue: "Intermittent failure in rpc_invalidateblock.py assert_equal(self.nodes[0].getblockchaininfo()['headers'], 7) [ AssertionError: not(24 == 7)]"
(https://github.com/bitcoin/bitcoin/issues/32965)

🚀 fanquake merged a pull request: "test: fix intermittent failure in rpc_invalidateblock.py"
(https://github.com/bitcoin/bitcoin/pull/32968)

🚀 fanquake merged a pull request: "test: headers sync timeout"
(https://github.com/bitcoin/bitcoin/pull/32677)

💬 marcofleon commented on pull request "p2p: improve TxOrphanage denial of service bounds":
(https://github.com/bitcoin/bitcoin/pull/31829#issuecomment-3073205784)
ReACK 50024620b909fc30b68a3715680e963f048482a5

A couple additional assertions, some nits addressed, and improvements in the `txorphanage_sim` fuzz target since last review. Ran the fuzz tests for a bit on existing corpora to be sure.

🤔 rkrux reviewed a pull request: "wallet: Construct ScriptPubKeyMans with all data rather than loaded progressively"
(https://github.com/bitcoin/bitcoin/pull/28333#pullrequestreview-3019765429)
Concept ACK 93c3729ad5adc02f6d87ec8daa1a1952d7c41009

This PR makes `DescriptorScriptPubKeyMan` class RAII compliant by adding 4 constructors that are responsible for allocating all the resources (eg: memory) required by this class during initialisation itself. I feel that it improves the readability quite a lot as the reader can easily gauge what all is needed by this class at one place and there is no need for multiple scattered class-write function calls like the one below that used to appe
...

💬 rkrux commented on pull request "wallet: Construct ScriptPubKeyMans with all data rather than loaded progressively":
(https://github.com/bitcoin/bitcoin/pull/28333#discussion_r2207211213)
In fdba094a67906cc89918661e24c9c9a5e3f4cb0b "wallet: Load everything into DescSPKM on construction"

```diff
- std::map<CKeyID, std::pair<CPubKey, std::vector<unsigned char>>> ckeys;
+ CryptedKeyMap ckeys;
```

💬 rkrux commented on pull request "wallet: Construct ScriptPubKeyMans with all data rather than loaded progressively":
(https://github.com/bitcoin/bitcoin/pull/28333#discussion_r2207137035)
In 32dd0e35251973f9bdc2114e70c800624894d489 "wallet: Consolidate generation setup callers into one function"

While this flow is being cleaned up. There is a check specially for this flag above on line 400 that forbids this situation and the secondary check seems redundant here now.

```diff
- if (!passphrase.empty() && !(wallet_creation_flags & WALLET_FLAG_DISABLE_PRIVATE_KEYS)) {
+ if (!passphrase.empty()) {
```

💬 rkrux commented on pull request "wallet: Construct ScriptPubKeyMans with all data rather than loaded progressively":
(https://github.com/bitcoin/bitcoin/pull/28333#discussion_r2207145704)
In https://github.com/bitcoin/bitcoin/commit/32dd0e35251973f9bdc2114e70c800624894d489 "wallet: Consolidate generation setup callers into one function"

Fine as-is but perhaps this block can be put inside the `if` block below because from what I see if not set by the user, this flag can only be set in case of `passphrase` not being empty on line 388. This wrapped inside a tighter constraint such as the one below looks better to me.

💬 rkrux commented on pull request "wallet: Construct ScriptPubKeyMans with all data rather than loaded progressively":
(https://github.com/bitcoin/bitcoin/pull/28333#discussion_r2207213178)
In fdba094a67906cc89918661e24c9c9a5e3f4cb0b "wallet: Load everything into DescSPKM on construction"

```diff
- std::map<CKeyID, CKey> keys;
+ KeyMap keys;
```

💬 rkrux commented on pull request "wallet: Construct ScriptPubKeyMans with all data rather than loaded progressively":
(https://github.com/bitcoin/bitcoin/pull/28333#discussion_r2207220067)
In fdba094a67906cc89918661e24c9c9a5e3f4cb0b "wallet: Load everything into DescSPKM on construction"

Some of them are now being used in `wallet.cpp` & `external_signer_scriptpubkeyman.h` and I suggested using couple in `walletdb.cpp` too. Maybe they can now go in a utility file like `walletutil.h`?

💬 rkrux commented on pull request "wallet: Construct ScriptPubKeyMans with all data rather than loaded progressively":
(https://github.com/bitcoin/bitcoin/pull/28333#discussion_r2207281849)
In https://github.com/bitcoin/bitcoin/commit/93c3729ad5adc02f6d87ec8daa1a1952d7c41009 "wallet: Setup new autogenerated descriptors on construction"

This will make `storage` the first argument in all the 4 constructors and this consistency would be nice for readability imo.
```diff
- DescriptorScriptPubKeyMan(WalletBatch& batch, WalletStorage& storage, WalletDescriptor& descriptor, int64_t keypool_size, const FlatSigningProvider& provider);
+ DescriptorScriptPubKeyMan(WalletStorage& storage
...

💬 rkrux commented on pull request "wallet: Construct ScriptPubKeyMans with all data rather than loaded progressively":
(https://github.com/bitcoin/bitcoin/pull/28333#discussion_r2207159782)
In https://github.com/bitcoin/bitcoin/commit/32dd0e35251973f9bdc2114e70c800624894d489 "wallet: Consolidate generation setup callers into one function"

This never seems to return `false`. Do we need to check for a `falsy` value while calling it like done above? Maybe not even return a bool? Just a thrown exception that is bubbled up.

💬 rkrux commented on pull request "wallet: Construct ScriptPubKeyMans with all data rather than loaded progressively":
(https://github.com/bitcoin/bitcoin/pull/28333#discussion_r2207275810)
In 93c3729ad5adc02f6d87ec8daa1a1952d7c41009 "wallet: Setup new autogenerated descriptors on construction"

```diff
- //! Create an automatically generated DescriptorScriptPubKeyMan
+ //! Create an automatically generated DescriptorScriptPubKeyMan (i.e. during wallet or descriptor creation)
```

💬 rkrux commented on pull request "wallet: Construct ScriptPubKeyMans with all data rather than loaded progressively":
(https://github.com/bitcoin/bitcoin/pull/28333#discussion_r2207233829)
In https://github.com/bitcoin/bitcoin/commit/fdba094a67906cc89918661e24c9c9a5e3f4cb0b "wallet: Load everything into DescSPKM on construction"

I feel these 2 can be kept inside `LegacyDataSPKM` itself, they are not used anywhere else, and I don't suppose they will be as well in the future.

💬 vasild commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2207333383)
> something like `doc/privacy`

Concept ACK. Out of the scope of this PR.