💬 ajtowns commented on pull request "log: print every script verification state change":
(https://github.com/bitcoin/bitcoin/pull/33336#discussion_r2455043810)
nit: Would be better to initialise to `nullptr`, IMO. Sanitizers should catch and warn for initialisation being missed in tested code paths though.

💬 diegoviola commented on pull request "Bugfix - don't allow multiple dialogs for same tx in TransactionView":
(https://github.com/bitcoin-core/gui/pull/817#issuecomment-3436839392)
> Temporarily drafting it since I've found some issues on Qt 6 while reviewing #904. WIP.

Can you say what those issues are?

💬 laanwj commented on pull request "util: Revert "common: Close non-std fds before exec in RunCommandJSON"":
(https://github.com/bitcoin/bitcoin/pull/33063#issuecomment-3436926890)
Note that strictly, the current (inefficient loop) subprocess implementation of fd closing in upstream isn't async signal safe as it calls `sysconf(_SC_OPEN_MAX)` in the child process ( https://github.com/arun11299/cpp-subprocess/blob/master/cpp-subprocess/subprocess.hpp#L1941 ).

Sadly:

> POSIX.1-2001 and POSIX.1-2001 TC2 required the functions fpathconf(3), pathconf(3), and sysconf(3) to be async-signal-safe, but this requirement was removed in POSIX.1-2008.

It also throws in case `sy
...

💬 laanwj commented on pull request "doc: add clarifying comment that creating the .bitcoin file is to avoid any mainnet datadir access":
(https://github.com/bitcoin/bitcoin/pull/33503#discussion_r2455142983)
This is not only about the mainnet datadir, but *any* default datadir. i'm not convinced that this comment adds any information beyond that in the previous line, which already states that.

💬 laanwj commented on pull request "init: Split file path handling out of -asmap option":
(https://github.com/bitcoin/bitcoin/pull/33631#issuecomment-3437071519)
> @laanwj do you agree this PR would be unnecessary if the ip_asn.map file were loaded by default (or if asmap data were embedded in the binary and enabled by default)? Those seem like cleaner solutions to me.

The reason i like this PR is that i dislike how the current code interprets an argument as either a boolean or a string. This is not compatible with stricter option parsing.

If reading a file by default or embedding it into the binary gets rid of that, that's fine with me too. But i
...

💬 laanwj commented on pull request "doc: better document NetEventsInterface and the deletion of "CNode"s":
(https://github.com/bitcoin/bitcoin/pull/32278#discussion_r2455209415)
What do you mean with "(without a help from the standard library)"?

💬 fanquake commented on pull request "miner: fix empty mempool case for waitNext()":
(https://github.com/bitcoin/bitcoin/pull/33566#issuecomment-3437145694)
Backported to `30.x` in #33609.

💬 achow101 commented on pull request "p2p: add `DifferenceFormatter` fuzz target and invariant check":
(https://github.com/bitcoin/bitcoin/pull/33252#issuecomment-3437158738)
ACK 65a10fc3c52ea09a4794345bcf607dff908c783a

💬 maflcko commented on pull request "ci: add Valgrind fuzz":
(https://github.com/bitcoin/bitcoin/pull/33461#issuecomment-3437177586)
Just noting that this is now the slowest test on the GitHub runners (outside this repo). With or without the cache, the runtime there seems to be 3h +- 5 minutes (https://github.com/maflcko/bitcoin-core-with-ci/actions/runs/18740251063/job/53473768619)

Not sure if we should aim to be below some runtime, just wanted to mention it could be mildly distracting to have to wait 3h to get a full CI result (and failed notification on error). Fine by me, but not sure if others rely on that CI pipeline
...

🤔 rkrux reviewed a pull request: "Split `CWallet::Create()` into `CreateNew` and `LoadExisting`"
(https://github.com/bitcoin/bitcoin/pull/32636#pullrequestreview-3369787513)
Code review b29d22a3d8395865a0a79daf16d44b4eb98f571e

Looks pretty neat to me, the refactor clearly differentiates between the wallet creation & load, and makes it easy to navigate this areas of the codebase. The usage of `PopulateWalletFromDB` function in `LoadExisting` and not in `CreateNew` is also helpful in maintaining the reading flow.
+1 on also fixing the encrypted wallet logging bug.

I believe this refactor is worth checking in the master branch.

💬 rkrux commented on pull request "Split `CWallet::Create()` into `CreateNew` and `LoadExisting`":
(https://github.com/bitcoin/bitcoin/pull/32636#discussion_r2455027884)
In 835de2490ed925da8df77815e8e1a6457bf1dc1e "refactor: wallet: Add `WriteVersion()`"

> Writing the wallet version needs to be done on wallet creation, but
WalletBatch::LoadWallet() does not, so factor this functionality out.

Note for the commit message: I had a tough time trying to parse this text and don't fully understand what it's supposed to say. As per code, ISTM that the wallet version is written unconditionally on creation and conditionally on load.

💬 rkrux commented on pull request "Split `CWallet::Create()` into `CreateNew` and `LoadExisting`":
(https://github.com/bitcoin/bitcoin/pull/32636#discussion_r2455069780)
In https://github.com/bitcoin/bitcoin/commit/835de2490ed925da8df77815e8e1a6457bf1dc1e "refactor: wallet: Add WriteVersion()"

Nit to differentiate this call from the other functions called above because this function is this class's member:

```diff
diff --git a/src/wallet/walletdb.cpp b/src/wallet/walletdb.cpp
index 41855b4154..387568282d 100644
--- a/src/wallet/walletdb.cpp
+++ b/src/wallet/walletdb.cpp
@@ -1175,7 +1175,7 @@ DBErrors WalletBatch::LoadWallet(CWallet* pwallet)

...

💬 rkrux commented on pull request "Split `CWallet::Create()` into `CreateNew` and `LoadExisting`":
(https://github.com/bitcoin/bitcoin/pull/32636#discussion_r2455240608)
In 79adf2163138cd1e30637904243945f191dc721d "scripted-diff: refactor: rename CWallet::LoadWallet"

`PopulateWalletFromDB` is a public function of class `CWallet` that is called by the outside modules such as tests and wallet-tool besides the static function `CWallet::LoadExisting`. As a reader going through these call sites does make me wonder for a bit what this function would be doing differently from the usual `Load` function, even though the name is quite self-explanatory to a large extent
...

💬 laanwj commented on pull request "test: sock: Enable socket pair tests on Windows":
(https://github.com/bitcoin/bitcoin/pull/33506#issuecomment-3437249052)
Concept ACK. Should we keep using `socketpair` on platforms that support it?

👍 rkrux approved a pull request: "doc: update multisig tutorial to use multipath descriptors"
(https://github.com/bitcoin/bitcoin/pull/33286#pullrequestreview-3370189629)
crACK de7c3587cd4586bbed94a4ea6eae4a252301daee

Sorry for missing out on the question earlier, I verified it using the `getdescriptorinfo` RPC for a multipath descriptor and it looks good to me.

💬 laanwj commented on pull request "test: sock: Enable socket pair tests on Windows":
(https://github.com/bitcoin/bitcoin/pull/33506#discussion_r2455303840)
Might want to check `blen == sizeof(bound)` after this as well

🤔 ajtowns reviewed a pull request: "net: option to disallow v1 connection on ipv4 and ipv6 peers"
(https://github.com/bitcoin/bitcoin/pull/30951#pullrequestreview-3369242392)
Concept review:

I think this has two potential benefits:

* avoids cleartext relay of transactions, particularly your own. #29415 is better at solving that problem, but a small step is also a good thing. this is only useful if you also disable inboud clearnet connections however.
* avoids easy detection that you're running a bitcoin node by stateless packet connection. if you're a listening node, then you still advertise your ip over the bitcoin network, so it's not hard to find you, in
...

💬 ajtowns commented on pull request "net: option to disallow v1 connection on ipv4 and ipv6 peers":
(https://github.com/bitcoin/bitcoin/pull/30951#discussion_r2454643475)
nit: Should check `DisableV10nClearnet()` first -- `ShouldReconnectV1` takes locks in order to return true.

💬 ajtowns commented on pull request "net: option to disallow v1 connection on ipv4 and ipv6 peers":
(https://github.com/bitcoin/bitcoin/pull/30951#discussion_r2454652805)
Shouldn't there be a constant with the default setting?

When do you "really need" this option? Might be better to give a more specific recommendation here?

If you usually want to set `-listen=0` with it, should we add an arg-interaction rather than just help text?

```c++
if (args.GetBoolArg("-v2onlyclearnet", false)) {
if (args.SoftSetBoolArg("-listen", false)) {
LogInfo("parameter interaction: -v2onlyclearnet set -> setting -listen=0\n");
}
}
```

💬 ajtowns commented on pull request "net: option to disallow v1 connection on ipv4 and ipv6 peers":
(https://github.com/bitcoin/bitcoin/pull/30951#discussion_r2455290421)
Maybe add an `IsClearnet(Network net) { return net == NET_IPV4 || net == IPV6; }` helper? There's a few other only-clearnet conditions in the code.