💬 josibake commented on pull request "Silent Payments: Implement BIP352":
(https://github.com/bitcoin/bitcoin/pull/28122#discussion_r1286941460)
this should be its own commit

💬 josibake commented on pull request "Silent Payments: Implement BIP352":
(https://github.com/bitcoin/bitcoin/pull/28122#discussion_r1286941957)
this should be its own commit, might not be needed if we instead V0SilentPaymentDestination as a CTxDestination type

📝 Crypt-iQ converted_to_draft a pull request: "p2p: ensure mapBlockSource is removed from in ProcessBlock"
(https://github.com/bitcoin/bitcoin/pull/28235)
If we receive a valid block that passes the anti-DoS threshold but doesn't advance the tip, the BlockChecked callback won't be called. This is because ActivateBestChain will return early since pindexMostWork is equal to m_chain.Tip(). Since the BlockChecked callback isn't called, mapBlockSource won't be removed from. Fix that by always removing from mapBlockSource in ProcessBlock.

💬 RicYashiroLee commented on pull request "Remove arbitrary restrictions on OP_RETURN by default":
(https://github.com/bitcoin/bitcoin/pull/28130#issuecomment-1669394875)
> Bitcoin is purely a layer for storage of data that, typically, represents financial transactions. We have witnessed for years that there is no reasonable way to prevent the storage of abitrary data (any system that allows user input of 'random' values should accept the inevitability of data storage, "the signal is the noise").
>
> The internet became the ubiquitous system that it is today not because some weak-willed soul decided for the users that they needed to be protected by limiting th
...

💬 petertodd commented on pull request "Remove -mempoolfullrbf option":
(https://github.com/bitcoin/bitcoin/pull/26525#issuecomment-1669412860)
> Not sure how all of you can be so comfortable with this kind of distortion of status quo, particularly when it serves no one but Peter Todd, some misguided wannabe Core Wizards, and whichever miners Peter can persuade to run code they dont really care about anyway.

FYI excluding Luxor, I haven't had any significant communications with any mining pools. I of course attempted to contact every mining pool I could find contact details for. But other than two I didn't get any responses back, and
...

👍 MarcoFalke approved a pull request: "crypto: more `Span<std::byte>` modernization & follow-ups"
(https://github.com/bitcoin/bitcoin/pull/28100#pullrequestreview-1535733421)
nice ACK 153a6745fa523a3fd0ccce1304ceb75d4511df20 🔖

<details><summary>Show signature</summary>

Signature:

```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: nice ACK 153a6745fa523a3f
...

💬 MarcoFalke commented on pull request "crypto: more `Span<std::byte>` modernization & follow-ups":
(https://github.com/bitcoin/bitcoin/pull/28100#discussion_r1267199359)
nit in 9e1199c7ce5d9a29c6ddf3dc28d8814bb1190a64: Can replace `std::byte{}` with `{}`, if it compiles, same below?

💬 MarcoFalke commented on pull request "crypto: more `Span<std::byte>` modernization & follow-ups":
(https://github.com/bitcoin/bitcoin/pull/28100#discussion_r1267206399)
9e1199c7ce5d9a29c6ddf3dc28d8814bb1190a64: Could replace this whole function with:

```cpp
fillrand(MakeWritableByteSpan(ret));

💬 MarcoFalke commented on pull request "crypto: more `Span<std::byte>` modernization & follow-ups":
(https://github.com/bitcoin/bitcoin/pull/28100#discussion_r1267215003)
same

💬 MarcoFalke commented on pull request "crypto: more `Span<std::byte>` modernization & follow-ups":
(https://github.com/bitcoin/bitcoin/pull/28100#discussion_r1267214464)
Any reason to do this? Fuzz inputs may already invalidate for any code change and maintaining compat for some code changes seems inconsistent, will bloat the fuzz code, and cause maintenance overhead in the future.

Would be good to either remove this, or if you *actually* want to maintain compat use a ternary operator:

```cpp
const auto key{fdp.ConsumeBool()?ConsumeFixedLen(...):std::vec<std::byte>(32)};

💬 MarcoFalke commented on pull request "crypto: more `Span<std::byte>` modernization & follow-ups":
(https://github.com/bitcoin/bitcoin/pull/28100#discussion_r1286960165)
2ff930de1c7f4683d0a7a72970683e21bf20ba73: SetKey. (no 32)

💬 MarcoFalke commented on pull request "crypto: more `Span<std::byte>` modernization & follow-ups":
(https://github.com/bitcoin/bitcoin/pull/28100#discussion_r1286968172)
nit in 2ff930de1c7f4683d0a7a72970683e21bf20ba73: Could use `{}` over `std::byte{}`, if it compiles?

💬 MarcoFalke commented on pull request "crypto: more `Span<std::byte>` modernization & follow-ups":
(https://github.com/bitcoin/bitcoin/pull/28100#discussion_r1286961781)
2ff930de1c7f4683d0a7a72970683e21bf20ba73: My preference would be to instead base this on a new commit that just does a `input` -> `m_input` ("scripted-diff") rename

💬 MarcoFalke commented on pull request "crypto: more `Span<std::byte>` modernization & follow-ups":
(https://github.com/bitcoin/bitcoin/pull/28100#discussion_r1286971030)
unrelated: In C++20, we'll probably have to provide overloads for `operator==(std::span, std::span)`, because equality checks are only defined in the std lib for containers that store memory, it seems.

✅ hebasto closed a pull request: "Avoid lock order inversion in `Chainstate::ConnectTip` function"
(https://github.com/bitcoin/bitcoin/pull/27684)

💬 TheCharlatan commented on pull request "refactor: Remove unused includes from wallet.cpp":
(https://github.com/bitcoin/bitcoin/pull/28200#issuecomment-1669476588)
Re https://github.com/bitcoin/bitcoin/pull/28200#issuecomment-1668087768

> Can you post a patch that compiles?

Seems like an easy win, no?

```diff
diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp
index 7c74b6b9ff..fe4c3d0f67 100644
--- a/src/wallet/wallet.cpp
+++ b/src/wallet/wallet.cpp
@@ -79,6 +79,8 @@
#include <tuple>
#include <variant>

+#include <boost/signals2/signal.hpp>
+
struct KeyOriginInfo;

using interfaces::FoundBlock;
diff --git a/src/wallet/w
...

💬 MarcoFalke commented on pull request "refactor: Remove unused includes from wallet.cpp":
(https://github.com/bitcoin/bitcoin/pull/28200#issuecomment-1669483448)
> Seems like an easy win, no?

No. The boost include is still there (via `src/wallet/scriptpubkeyman.h:#include <boost/signals2/signal.hpp>`

💬 ChrisCho-H commented on pull request "fix: bad opcode err msg includes reserved opcode":
(https://github.com/bitcoin/bitcoin/pull/28234#issuecomment-1669509083)
Thx, I updated in same commit.

💬 Daniel600 commented on pull request "policy: Enable full-rbf by default":
(https://github.com/bitcoin/bitcoin/pull/28132#issuecomment-1669514712)
For accuracy purposes GAP600 supports a range of clients who use 0-conf for their end users in a range of applications - including gaming/gambling, instant crypto swops, and Forex. All industries which take their privacy very seriously. GAP600 has provided email quotes and root addresses (here and in the mailing list) in order to validate this claim for this signifiant market.

You have accused GAP600 and myself of outright lying, based on your perceived understanding of how GAP600 should run
...

💬 Crypt-iQ commented on pull request "p2p: ensure mapBlockSource is removed from in ProcessBlock":
(https://github.com/bitcoin/bitcoin/pull/28235#issuecomment-1669520073)
I guess I forgot to run the functional tests - the issue is that this patch removes from `mapBlockSource` in `ProcessBlock` if `new_block` is set. I assumed that this meant it was a valid block, but that's not always the case. With this patch, if we receive more blocks on top of this invalid block that give us more work than the tip, the `BlockChecked` callback will run and won't find the peer to punish. I can't think of a scenario where you're sure that you can remove from `mapBlockSource` unle
...