💬 Sjors commented on pull request "net, net_processing: additional and consistent disconnect logging":
(https://github.com/bitcoin/bitcoin/pull/28521#discussion_r1858445736)
I added a commit to improve this message.

💬 Sjors commented on pull request "net, net_processing: additional and consistent disconnect logging":
(https://github.com/bitcoin/bitcoin/pull/28521#issuecomment-2500659531)
Rebased and addressed feedback.

💬 Sjors commented on pull request "net, net_processing: additional and consistent disconnect logging":
(https://github.com/bitcoin/bitcoin/pull/28521#discussion_r1858447297)
Fixed

💬 jonatack commented on pull request "wallet: Keep track of the wallet's own transaction outputs in memory":
(https://github.com/bitcoin/bitcoin/pull/27286#issuecomment-2500679564)
Concept ACK, if this patchset can meaningfully improve performance and reliably refresh wallet utxos to keep the in-memory cache in sync.

💬 jonatack commented on pull request "wallet: Track no-longer-spendable TXOs separately":
(https://github.com/bitcoin/bitcoin/pull/27865#issuecomment-2500682230)
Concept ACK, pending https://github.com/bitcoin/bitcoin/pull/27286, if this improves performance for wallets with many transactions.

💬 maflcko commented on pull request "ci: Test cross-built Windows executables on Windows natively":
(https://github.com/bitcoin/bitcoin/pull/31176#discussion_r1858472581)
remove wine?

💬 maflcko commented on pull request "ci: Test cross-built Windows executables on Windows natively":
(https://github.com/bitcoin/bitcoin/pull/31176#discussion_r1858474155)
Would be nice to run the util and functional tests as well

👍 maflcko approved a pull request: "ci: Test cross-built Windows executables on Windows natively"
(https://github.com/bitcoin/bitcoin/pull/31176#pullrequestreview-2461424433)
lgtm

💬 theStack commented on pull request "doc, test: more ephemeral dust follow-ups":
(https://github.com/bitcoin/bitcoin/pull/31371#discussion_r1858489166)
Note that this fee is set initially for a tx with a single (non-dust) output, and any additional outputs after are created in a way that the total tx fee stays constant (see doc-string of function `add_output_to_create_multi_result`: `Add output without changing absolute tx fee`). So the parameter denotes indeed the fee for the whole tx.

💬 jonatack commented on pull request "wallet: Be able to receive and spend inputs involving MuSig2 aggregate keys":
(https://github.com/bitcoin/bitcoin/pull/29675#issuecomment-2500731539)
Concept ACK

> Given BIP373 doesn't have test vectors, it would be very useful that either this PR or the BIP include some hard coded PSBT examples to ensure every implementations are on the same page.

Good point (the BIP373 test vector section currently states "TBD" and seems worth completing even if the implementation here also has tests).

💬 stickies-v commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r1858493399)
> You'd probably want to pass this function a script verify object that has already passed through the required pre-checks.

I 100% agree with this approach. Adding extra types makes the API more cumbersome to use, but I think it does make it more safe, and the extra verbosity should be quite easy to hide in client libraries.

> But then you're forced to check them here again.

I think that can be avoided by having the prechecks function return a `ScriptPreChecksPassed*` (which references
...

💬 theStack commented on pull request "doc, test: more ephemeral dust follow-ups":
(https://github.com/bitcoin/bitcoin/pull/31371#discussion_r1858493647)
Hmm, thought about that, but I think this could be confusing, especially considering that transactions are still created with nVersion=2 by default (see e.g. https://github.com/bitcoin/bitcoin/issues/31348). Seems more expressive to be explicit about the tx version.

💬 theStack commented on pull request "doc, test: more ephemeral dust follow-ups":
(https://github.com/bitcoin/bitcoin/pull/31371#discussion_r1858494074)
Will do if I have to retouch.

💬 Sjors commented on pull request "net, net_processing: additional and consistent disconnect logging":
(https://github.com/bitcoin/bitcoin/pull/28521#issuecomment-2500740657)
Fixed [tidy complaint](https://cirrus-ci.com/task/4855637697363968?logs=ci#L3329-L3332).

💬 1440000bytes commented on pull request "ci: detect outbound internet traffic generated while running tests":
(https://github.com/bitcoin/bitcoin/pull/31349#issuecomment-2500761664)
Concept ACK

A simple solution to avoid leaking IP address when running tests locally would be to disconnect internet while running tests.

🤔 jonatack reviewed a pull request: "wallet, desc spkm: Return SigningProvider only if we have the privkey"
(https://github.com/bitcoin/bitcoin/pull/31242#pullrequestreview-2461503357)
ACK modulo test coverage, a few suggestions if you repush.

💬 jonatack commented on pull request "wallet, desc spkm: Return SigningProvider only if we have the privkey":
(https://github.com/bitcoin/bitcoin/pull/31242#discussion_r1858519759)
```suggestion
bool FlatSigningProvider::HaveKey(const CKeyID& keyid) const
```

💬 jonatack commented on pull request "wallet, desc spkm: Return SigningProvider only if we have the privkey":
(https://github.com/bitcoin/bitcoin/pull/31242#discussion_r1858520727)
```suggestion
std::unique_ptr<FlatSigningProvider> out = GetSigningProvider(index, /*include_private=*/true);
```

💬 jonatack commented on pull request "wallet, desc spkm: Return SigningProvider only if we have the privkey":
(https://github.com/bitcoin/bitcoin/pull/31242#discussion_r1858519981)
```suggestion
bool HaveKey(const CKeyID& keyid) const override;
```

💬 TheCharlatan commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r1858539457)
I'll propose a pre-check function and object later today then :)