💬 michaelfolkson commented on pull request "MiniTapscript: port Miniscript to Tapscript":
(https://github.com/bitcoin/bitcoin/pull/27255#issuecomment-1473777054)
> Has there been any thought into how MuSig2 (and threshold friends) would fit into this in the future?

Discussed in sipa's [gist](https://gist.github.com/sipa/06c5c844df155d4e5044c2c8cac9c05e#musig-descriptors). Can comment on that gist @Sjors

💬 MarcoFalke commented on issue "Increase fuzz coverage in the wallet":
(https://github.com/bitcoin/bitcoin/issues/27272#issuecomment-1473862770)
See https://github.com/bitcoin/bitcoin/pull/23444

💬 stickies-v commented on pull request "httpserver, rest: fix segmentation fault on evhttp_uri_get_query":
(https://github.com/bitcoin/bitcoin/pull/27253#discussion_r1140271032)
What do you think about this approach? I think it's quite minimal:

<details>
<summary>git diff</summary>

```diff
diff --git a/src/httpserver.cpp b/src/httpserver.cpp
index 942caa042..6a6176a78 100644
--- a/src/httpserver.cpp
+++ b/src/httpserver.cpp
@@ -15,6 +15,7 @@
#include <rpc/protocol.h> // For HTTP status codes
#include <shutdown.h>
#include <sync.h>
+#include <util/check.h>
#include <util/strencodings.h>
#include <util/syscall_sandbox.h>
#include <util/system.h>

...

💬 Bushstar commented on pull request "refactor: remove unused param from legacy pubkey interface":
(https://github.com/bitcoin/bitcoin/pull/27274#discussion_r1140293524)
Updated.

💬 mzumsande commented on pull request "test: Make the unlikely race in p2p_invalid_messages impossible":
(https://github.com/bitcoin/bitcoin/pull/27212#issuecomment-1473940615)
ACK fa1eb0ecaef14d428812f956082d29ab134fc728

👍 pinheadmz approved a pull request: "test: Make the unlikely race in p2p_invalid_messages impossible"
(https://github.com/bitcoin/bitcoin/pull/27212)
ACK fa1eb0ecaef14d428812f956082d29ab134fc728

Thanks @mzumsande for the explanation, I compared the logs of failed and successful tests and I can see the message order is fixed

<details><summary>Show Signature</summary>

```
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256

ACK fa1eb0ecaef14d428812f956082d29ab134fc728
-----BEGIN PGP SIGNATURE-----

iQIzBAEBCAAdFiEE5hdzzW4BBA4vG9eM5+KYS2KJyToFAmQUfpsACgkQ5+KYS2KJ
yTrJUg//TlRg8JalWfEd+Rb2hklRFFJpE7PcaEajr5in71WlXaCQ9mCRaXHNtkI5
XepC
...

🚀 fanquake merged a pull request: "test: Make the unlikely race in p2p_invalid_messages impossible"
(https://github.com/bitcoin/bitcoin/pull/27212)

💬 pinheadmz commented on pull request "test: fix intermittent issue in `feature_bip68_sequence`":
(https://github.com/bitcoin/bitcoin/pull/27177#issuecomment-1473999269)
The filtering logic looks very nice and clean now! But, so, are there only those two mempool tests that need immature spends? It looks like you had to add lots of argument-forwarding to get there. I'm wondering if you've looked into the possibility of just adding more block confirmations in those two tests? And then you perhaps wouldn't need `avoid_immature_coinbase` at all which I feel should just be an implied default anyway.

💬 MarcoFalke commented on pull request "refactor / kernel: Move non-gArgs chainparams functionality to kernel":
(https://github.com/bitcoin/bitcoin/pull/26177#discussion_r1140383664)
> what(): CreateChainParams: Unknown chain lol

I think you tested the wrong thing. For me it prints:

```
Error: SigNetParams: -signetchallenge cannot be multiple values.
```

Which is confusing to an end-user, because it leaks internal implementation details that are irrelevant and redundant to the user.

💬 MarcoFalke commented on pull request "refactor / kernel: Move non-gArgs chainparams functionality to kernel":
(https://github.com/bitcoin/bitcoin/pull/26177#discussion_r1140384430)
To test, `-signetchallenge=nonhex` silently fails/passes

💬 Xekyo commented on pull request "wallet, tests: Expand and test when the blank wallet flag should be un/set":
(https://github.com/bitcoin/bitcoin/pull/25634#discussion_r1140387995)
You are explicitly expecting this behavior, but I’m kinda surprised that when we import a wallet that was explicitly created “blank” after the import it is now no longer blank. As far as I can tell, no keys were created, unless the import automatically does that. I’m curious why this is the expected behavior (although since you seem to explicitly expect that behavior, I don’t think there is an issue here).

💬 Xekyo commented on pull request "wallet, tests: Expand and test when the blank wallet flag should be un/set":
(https://github.com/bitcoin/bitcoin/pull/25634#discussion_r1140372731)
Using “not” in the context of a boolean feels always a bit like a double negation. Perhaps go with “without”:

```suggestion
{RPCResult::Type::BOOL, "blank", "whether this wallet was intentionally created without keys, scripts, or descriptors"},
```

💬 ryanofsky commented on pull request "rpc, test: remove newline escape sequence from wallet warning fields":
(https://github.com/bitcoin/bitcoin/pull/27138#issuecomment-1474031095)
> The newline is presentation-specific and does not belong to the server output.

Again, for the record, I disagree. The RPC server does not return HTML strings or XML strings, it returns text strings, and the newline character has a very clear meaning in text strings.

Newline is a better delimiter than space if there are multiple warnings, so they are separable and coherent. Replacing newlines with spaces produces addled run-on paragraphs like "Wallet will not be encrypted. Wallet created
...

💬 TheCharlatan commented on pull request "refactor, kernel: Decouple ArgsManager from blockstorage":
(https://github.com/bitcoin/bitcoin/pull/27125#issuecomment-1474048274)
Updated 593a9f36609a74f47175e681a3921f3975272766 -> 54d8644dce180197fa657e9b68d6de63cf4c8072 ([removeBlockstorageArgs_8](https://github.com/TheCharlatan/bitcoin/tree/removeBlockstorageArgs_8) -> [removeBlockstorageArgs_9](https://github.com/TheCharlatan/bitcoin/tree/removeBlockstorageArgs_9), [compare](https://github.com/TheCharlatan/bitcoin/compare/removeBlockstorageArgs_8..removeBlockstorageArgs_9)) to improve commit messages and add a commit adding the `blockmanager_args.cpp` file to iwyu as
...

🕺 restarted server sorry

💬 achow101 commented on pull request "wallet: return error msg for "too-long-mempool-chain"":
(https://github.com/bitcoin/bitcoin/pull/24845#issuecomment-1474452399)
ACK f3221d373a8623fe4808e00c7a92fbb6e0d6419b

💬 furszy commented on pull request "bumpfee: Allow the user to choose which output is change":
(https://github.com/bitcoin/bitcoin/pull/26467#discussion_r1140734813)
nit:
double unneeded `()`

💬 furszy commented on pull request "bumpfee: Allow the user to choose which output is change":
(https://github.com/bitcoin/bitcoin/pull/26467#discussion_r1140761441)
There still is a problem with this approach when the user sets a value for the single output.

Basically, the user provided value is being discarded. We don't check if it's within the inputs amount bounds nor if need to create a change output for the remaining amount (value can be lower than the inputs amount).

Current code instead of create a change output when the value is lower than the inputs amount, it sets the value equal to inputs total minus new fee. We should code a test for it too
...

👍 furszy approved a pull request: "wallet: Pass through transaction locktime and preset input sequences and scripts to CreateTransaction"
(https://github.com/bitcoin/bitcoin/pull/25273)
diff re-ACK 35fe41f

💬 achow101 commented on pull request "refactor: wallet, do not translate init options names":
(https://github.com/bitcoin/bitcoin/pull/25666#issuecomment-1474477293)
ACK e43a547a3674a31504a60ede9b4912e014a54139