👍 vasild approved a pull request: "rpc, test: remove newline escape sequence from wallet warning fields"
(https://github.com/bitcoin/bitcoin/pull/27138)
ACK f0391cd3ea562d526db63996e7694ff4cd44b8b2

The newline is presentation-specific and does not belong to the server output. For example it will not render as expected if displayed in HTML (where newlines are displayed as a single space). Then it should be `<br/>` instead of `\n`, or maybe `\r\n` for MacOS?

I am happy to review more of the same in another PR or a change that returns an array of warnings so that display engines can chose the best separator for them.

💬 hebasto commented on issue "When opening or autoloading wallets there should be clear messages about rescanning in progress and wallets' names.":
(https://github.com/bitcoin-core/gui/issues/259#issuecomment-1473654097)
@AdarshRawat1
> can I work on this issue?

This project is permissionless.

Feel free to open a PR :)

💬 BitcoinErrorLog commented on pull request "Remove -mempoolfullrbf option":
(https://github.com/bitcoin/bitcoin/pull/26525#issuecomment-1473707197)
> Philosophical arguments about full RBF aside, this PR proposes deleting an option that is (1) used by a significant amount of nodes and (2) does no harm to the node operator. It would be inappropriate to simply take such an option out from underneath users/downstream software without very strong rationale and a proper deprecation cycle.

Where were you when this was happening to first-seen 0conf users? Where was the very strong rationale and proper deprecation cycle then?

Separately, wha
...

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