💬 Bushstar commented on pull request "refactor: remove unused param from legacy pubkey interface":
(https://github.com/bitcoin/bitcoin/pull/27274#issuecomment-1473761343)
@Sjors There's three uses of internal so defining a local bool seemed cleaner. Update all three locations in the style you linked or leave it as it is?

💬 Bushstar commented on pull request "refactor: remove unused param from legacy pubkey interface":
(https://github.com/bitcoin/bitcoin/pull/27274#discussion_r1140178190)
There's three uses of internal so defining a local bool seemed cleaner. Update all three locations in the style you linked or leave it as it is?

💬 Sjors commented on pull request "MiniTapscript: port Miniscript to Tapscript":
(https://github.com/bitcoin/bitcoin/pull/27255#issuecomment-1473771519)
Very cool. I also noticed [bitcoin.sipa.be/miniscript](https://bitcoin.sipa.be/miniscript/) has been updated.

Has there been any thought into how MuSig2 (and friends) would fit into this in the future? I guess that's only a problem for the compiler, since for the purpose of script it doesn't matter if e.g. public key C is an aggregate of A + B. But it does change the meaning of something like `or(C,and(multi_a(A,B),after(10))`.

💬 ismaelsadeeq commented on pull request "test: Support decoding segwit address in address_to_scriptpubkey()":
(https://github.com/bitcoin/bitcoin/pull/27269#issuecomment-1473772572)
Thanks for reviewing, I made the update.

👍 brunoerg approved a pull request: "p2p: Improve diversification of new connections"
(https://github.com/bitcoin/bitcoin/pull/27264)
crACK 72e8ffd7f8dbf908e65da6d012ede914596737ab

💬 ajtowns commented on pull request "Remove -mempoolfullrbf option":
(https://github.com/bitcoin/bitcoin/pull/26525#issuecomment-1473775491)
> this PR proposes deleting an option that .. (2) does no harm to the node operator.

This isn't true. Running a non-default mempool makes it easier to put different txs in your mempool compared to the majority of the network, which:

- makes it easier to fingerprint your node and its network peers
- creates pinning vectors
- can allow your counterparties to hide unconfirmed payments to you or hide double-spends of payments to you
- lowers the effectiveness of compact block relay

A
...

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