💬 Sjors commented on pull request "refactor: remove unused param from legacy pubkey interface":
(https://github.com/bitcoin/bitcoin/pull/27274#discussion_r1140141439)
The following style is more consistent with what we do in other places:
'''cpp
if (!CanGetAddresses(/*internal*/=false)) {
``

💬 Sjors commented on pull request "refactor: remove unused param from legacy pubkey interface":
(https://github.com/bitcoin/bitcoin/pull/27274#issuecomment-1473738260)
Concept ACK

💬 ismaelsadeeq commented on pull request "test: Support decoding segwit address in address_to_scriptpubkey()":
(https://github.com/bitcoin/bitcoin/pull/27269#issuecomment-1473738719)
Yes, I agree the todo can be removed since there are no other address types to add.

- I think there is no need to add `byte_to_bech32()`, there is an existing encode_segwit_address() function that does the same thing.
- I can use bech32_to_bytes() and encode_segwit_address() with hex encoded string and version as test vectors in test_bech32_decode() just like in test_base58encodedecode().

💬 Sjors commented on pull request "Ignore datacarrier limits for dataless OP_RETURN outputs":
(https://github.com/bitcoin/bitcoin/pull/27261#issuecomment-1473745636)
I'm not sure how this is being used in practice, so perhaps the safest thing to do is what @ajtowns suggests: document clearly that `-datacarriersize=1` means "OP_RETURN allowed, but no data" whereas `-nodatacarrier` means "no OP_RETURN allowed at all".

💬 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