📝 fanquake locked a pull request: "Remove arbitrary restrictions on OP_RETURN by default"
(https://github.com/bitcoin/bitcoin/pull/28130)
Any number or size of data-carrying OP_RETURN outputs are allowed, and the `-datacarrier` option is removed. For those who want to limit data carrying outputs, `-datacarriersize` is still supported, and has the functionality of applying the specified data carrier limit as well as limiting the number of data carrying OP_RETURN outputs to one. If `-datacarriersize=0` is set, no data carrying output is allowed.

Rational: there's lots of ways for people to publish data in the Bitcoin chain, a
...

💬 dergoegge commented on pull request "util: Type-safe transaction identifiers":
(https://github.com/bitcoin/bitcoin/pull/28107#discussion_r1287123567)
Yea that works, applied in the latest push

💬 dergoegge commented on pull request "util: Type-safe transaction identifiers":
(https://github.com/bitcoin/bitcoin/pull/28107#discussion_r1287123814)
removed

💬 dergoegge commented on pull request "util: Type-safe transaction identifiers":
(https://github.com/bitcoin/bitcoin/pull/28107#discussion_r1287124882)
I kinda like making this explicit but the assignment operators are not necessary so i removed them

💬 dergoegge commented on pull request "util: Type-safe transaction identifiers":
(https://github.com/bitcoin/bitcoin/pull/28107#discussion_r1287125133)
Done

💬 dergoegge commented on pull request "util: Type-safe transaction identifiers":
(https://github.com/bitcoin/bitcoin/pull/28107#discussion_r1287125465)
Done

💬 dergoegge commented on pull request "util: Type-safe transaction identifiers":
(https://github.com/bitcoin/bitcoin/pull/28107#discussion_r1287125880)
Done

💬 instagibbs commented on pull request "p2p: ensure mapBlockSource is removed from in ProcessBlock":
(https://github.com/bitcoin/bitcoin/pull/28235#issuecomment-1669693642)
Since I think this behavior is fairly long-running, I think I might try to recap the current logic:

Every time a compact block is reconstructed, or full block directly given, we put an entry in a map for one of two reasons:
1) reward if they're a good high-bandwidth peer candidates because they gave us a new most-PoW block
2) punish if they gave us a full block and it has any fundamental issue, or if they handed us a compact block they should have known was faulty(e.g., invalid header)

I
...

💬 MarcoFalke commented on pull request "util: Type-safe transaction identifiers":
(https://github.com/bitcoin/bitcoin/pull/28107#discussion_r1287209576)
Apart from compare, assignment isn't type safe either. For example:

```cpp
uint256 b = Txid{};
Wtxid a {b};
```

compiles.

So my preference would be to add a comment that this is allowed, and remove this code that tries to imply the opposite and attempts to trick reviewers.

Alternatively it could be disallowed properly.

👍 TheCharlatan approved a pull request: "refactor: Remove unused includes from wallet.cpp"
(https://github.com/bitcoin/bitcoin/pull/28200#pullrequestreview-1567463775)
ACK fac3e2fa6adf9cb034ea5ee28c572b25cc7e3c2d

Re https://github.com/bitcoin/bitcoin/pull/28200#issuecomment-1669483448

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

Yeah, totally. Should have checked this before trusting the iwyu suggestion.

💬 Crypt-iQ commented on pull request "p2p: ensure mapBlockSource is removed from in ProcessBlock":
(https://github.com/bitcoin/bitcoin/pull/28235#issuecomment-1669750122)
Yup that makes sense

💬 instagibbs commented on pull request "test: tx orphan handling":
(https://github.com/bitcoin/bitcoin/pull/28199#discussion_r1287224406)
Could you say that in the test then? This really throws me off as a reader!

💬 instagibbs commented on pull request "test: tx orphan handling":
(https://github.com/bitcoin/bitcoin/pull/28199#issuecomment-1669753053)
ACK https://github.com/bitcoin/bitcoin/pull/28199/commits/abe8536192c9f2cd6ba9d0e083f23dec4d20841f

https://github.com/bitcoin/bitcoin/pull/28199#discussion_r1283271742 would be nice to be slightly cleaned up for future readers

💬 glozow commented on pull request "test: tx orphan handling":
(https://github.com/bitcoin/bitcoin/pull/28199#issuecomment-1669788993)
thanks for the concept acks @dergoegge @jamesob @brunoerg @Empact @jonatack, would appreciate a review of test too :pray:

💬 fjahr commented on pull request "Add ASM optimizations for MuHash3072":
(https://github.com/bitcoin/bitcoin/pull/19181#issuecomment-1669793604)
> There hasn't been much activity lately. What is the status here?
>
> [Finding reviewers](https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#finding-reviewers) may take time. However, if the patch is no longer relevant, please close this pull request. If the author lost interest or time to work on this, please close it and mark it 'Up for grabs' with the label, so that it can be picked up in the future.

Still relevant... How good is your ASM, @DrahtBot ? 😁

💬 fanquake commented on pull request "guix: use GCC 12.3.0 to build releases":
(https://github.com/bitcoin/bitcoin/pull/27897#issuecomment-1669797798)
Upstreamed a patch to (re-)add `powerpc64-linux` as a platform definition in Guix: https://debbugs.gnu.org/cgi/bugreport.cgi?bug=65151. Using a self-compiled Guix with this patch, fixes building our cross-compilation toolchain for `powerpc64-linux-gnu`, and means we'd just drop 26ee1fc5998cce6808f022fd1a4e1764115c2d57 from this PR.

Also pushed a patch to update mingw-w64 to `11.0.1`: https://lists.gnu.org/archive/html/guix-patches/2023-08/msg00299.html.

💬 Sjors commented on pull request "Silent Payments: Implement BIP352":
(https://github.com/bitcoin/bitcoin/pull/28122#issuecomment-1669800569)
Maybe point out in the description how the first commit 8283ca76e4feb37d3514fbd8dc491b8c4965dff1 relates to BIP 324, which doesn't use the ecdh module, but ElligatorSwift instead.

💬 furszy commented on pull request "p2p: adaptive connections services flags":
(https://github.com/bitcoin/bitcoin/pull/28170#discussion_r1287286613)
Have thought more about this and I'm not so convinced anymore.

The connection manager class lives on a lower level, and manages only a fraction of the node's information. And this field is strictly linked to the node's overall status. Right now, `HasDesirableServiceFlags()` requires to know the chain sync status.
So, by placing it inside `CConMan`, we add another level of indirection. Because, most of the time, the peer manager class will call to the `CConMan` setter and subsequently call th
...

👍 TheCharlatan approved a pull request: "refactor: Remove unused boost signals2 from torcontrol"
(https://github.com/bitcoin/bitcoin/pull/28240#pullrequestreview-1567496979)
ACK fa32af22b323e7c58b6b20af6517f4795a72cdc5

The `async_handler` was unused since its introduction, so I don't think it's worth it to keep it around.

💬 TheCharlatan commented on pull request "refactor: Remove unused boost signals2 from torcontrol":
(https://github.com/bitcoin/bitcoin/pull/28240#discussion_r1287241422)
Style nit: These pragmas could be indented like they are here: https://github.com/bitcoin/bitcoin/blob/master/src/util/strencodings.h#L20 , or did you omit any automatic formatting to maintain order?