👍 Sjors approved a pull request: "Ignore datacarrier limits for dataless OP_RETURN outputs"
(https://github.com/bitcoin/bitcoin/pull/27261#pullrequestreview-1463048048)
Concept ACK, unless there's clear historical context somewhere (that I missed) showing that the folks who introduced `-nodatacarrier` intended to prevent burning of coins (as distinct from "spam").

As far as I can tell the current behavior was implemented by accident in #5077. The goal of that PR was to make the limit configurable for miners. Specifically the limit at the time was 40 and some miners wanted 80. A side-effect of the change is that if you set it to 0 it would also stop OP_RETUR
...

💬 Sjors commented on pull request "Ignore datacarrier limits for dataless OP_RETURN outputs":
(https://github.com/bitcoin/bitcoin/pull/27261#discussion_r1218380058)
Note to other reviewers: `max_datacarrier_bytes` is nullptr when `DEFAULT_ACCEPT_DATACARRIER` is (patched to) `false`.

👍 ryanofsky approved a pull request: "Use `int32_t` type for most transaction size/weight values"
(https://github.com/bitcoin/bitcoin/pull/23962#pullrequestreview-1463079333)
Code review ACK 93daff4b4577a07994b57218df9bb83bed7bf743

💬 ryanofsky commented on pull request "Use `int32_t` type for most transaction size/weight values":
(https://github.com/bitcoin/bitcoin/pull/23962#discussion_r1218400476)
In commit "Remove txmempool implicit-integer-sign-change sanitizer suppressions" (93daff4b4577a07994b57218df9bb83bed7bf743)

IIUC, the code here casting `modifyCount` to be unsigned and adding it even though `modifyCount` might be negative is safe because the standard guarantees that:

1. Casting `modifyCount` to `uint64_t` produces an equivalent value mod 2⁶⁴
2. Adding two `uint64_t` values is performed mod 2⁶⁴

Also, the new code is equivalent to previous code beca
...

💬 hebasto commented on pull request "build: Restrict check for CRC32C intrinsic to aarch64":
(https://github.com/bitcoin/bitcoin/pull/23045#issuecomment-1577230185)
> Should it be submitted to the [upstream](https://github.com/google/crc32c/blob/21fc8ef30415a635e7351ffa0e5d5367943d4a94/CMakeLists.txt#L147-L156)?

> Maybe! If they have the same check, and haven't actually fixed ARM32 support, they'll have the same problem.

Done in https://github.com/google/crc32c/pull/61.

💬 Xekyo commented on pull request "fuzz: Fix mini_miner_selection running out of coin":
(https://github.com/bitcoin/bitcoin/pull/27806#discussion_r1218417470)
@achow101: I believe that it could not dip below 1 available input, since there were always two created and (n-1) added to the available_coins. Now, it should be impossible for the count to dip below the initial count because it consumes a maximum of two inputs and also adds at least two coins to available_coins every iteration.

Happy to add an assert, though

💬 sipa commented on pull request "fuzz: Fix mini_miner_selection running out of coin":
(https://github.com/bitcoin/bitcoin/pull/27806#issuecomment-1577245483)

💬 Xekyo commented on pull request "fuzz: Fix mini_miner_selection running out of coin":
(https://github.com/bitcoin/bitcoin/pull/27806#discussion_r1218422493)
@brunoerg: I think that would work as well, although it would probably lead to smaller clusters in some of the fuzzing

💬 miketwenty1 commented on issue "bitcoin-cli requires unnecessary data field for certain commands":
(https://github.com/bitcoin/bitcoin/issues/27828#issuecomment-1577252529)
After chatting with @sipa here https://github.com/bitcoin/bitcoin/pull/27829
The original bug/request of making "data" required is not the right move. I believe this may still be a bug with (json object) not being noted as required or optional.

This seems to be more of a display issue with `RPCArg::Type::OBJ` and the corresponding `RPCArg::Optional::OMITTED` perhaps?

https://github.com/bitcoin/bitcoin/blob/master/src/rpc/rawtransaction.cpp#L160
https://github.com/bitcoin/bitcoin/blob/mas
...

💬 pinheadmz commented on pull request "net: call getaddrinfo() in detachable thread to prevent stalling":
(https://github.com/bitcoin/bitcoin/pull/27557#issuecomment-1577271267)
> Needs rebase?

ty. 🤞

💬 miketwenty1 commented on issue "bitcoin-cli output help text - optional (json object)":
(https://github.com/bitcoin/bitcoin/issues/27828#issuecomment-1577273775)
Unsure why https://github.com/bitcoin/bitcoin/blob/master/src/rpc/util.cpp#L853 this isn't triggering.
Happy to work on this with maybe a guiding hand.

💬 pinheadmz commented on issue "Duplicate coinbase transaction space reservation in CreateNewBlock":
(https://github.com/bitcoin/bitcoin/issues/21950#issuecomment-1577337474)
This is actually pretty interesting: I quickly looked through the weight of the last 50-60 blocks and F2Pool is the ONLY pool generating blocks with total weight higher greater than 3,996,000. I started writing a unit test for this, it seems like that double coinbase reservation may actually put miners who rely on bitcoin core template generation at a disadvantage.

👍 theStack approved a pull request: "mempool / rpc: add getprioritisedtransactions, delete a mapDeltas entry when delta==0"
(https://github.com/bitcoin/bitcoin/pull/27501#pullrequestreview-1463232694)
Code-review ACK 67b7fecacd0489809690982c89ba2d0acdca938c

💬 theStack commented on pull request "mempool / rpc: add getprioritisedtransactions, delete a mapDeltas entry when delta==0":
(https://github.com/bitcoin/bitcoin/pull/27501#discussion_r1218496768)
Have no strong opinion on that either, just wanted to make sure that there wasn't some code using `modified_fee` field around that was maybe forgotten to be pushed or sth alike.

💬 amitiuttarwar commented on pull request "p2p: Diversify automatic outbound connections with respect to networks":
(https://github.com/bitcoin/bitcoin/pull/27213#discussion_r1218526503)
nice simplification! incorporated in latest push

💬 amitiuttarwar commented on pull request "p2p: Diversify automatic outbound connections with respect to networks":
(https://github.com/bitcoin/bitcoin/pull/27213#discussion_r1218543773)
yeah, I see what you're saying - that we can avoid iterating through all peers when we are only running on one network.

I've implemented a version of your suggestion in https://github.com/amitiuttarwar/bitcoin/commit/1805698096ae29e8dd6ecd1be9348a04f612602c. I changed the function implementation and also invoked from the additional call site in `MaybePickPreferredNetwork`. It seems viable.

I haven't pushed it here yet because I'm still trying to figure out a slightly cleaner solution. I n
...

💬 amitiuttarwar commented on pull request "p2p: Diversify automatic outbound connections with respect to networks":
(https://github.com/bitcoin/bitcoin/pull/27213#issuecomment-1577424366)
thanks for the review @brunoerg ! latest push incorporated fuzz test & `ThreadOpenConnections` simplification. I'm still working on the other feedback (as explained above).

one question about your suggested patch in the fuzz test:
`const Network net{fuzzed_data_provider.PickValueInArray({Network::NET_IPV4, Network::NET_IPV6, Network::NET_INTERNAL, Network::NET_ONION})};`
is there a reason to select from the subset of networks? in the patch I pushed up, I also added the other ones. I saw t
...

👍 ryanofsky approved a pull request: "wallet, tests: Expand and test when the blank wallet flag should be un/set"
(https://github.com/bitcoin/bitcoin/pull/25634#pullrequestreview-1463212375)
Code review ACK 7baa0971527ba3c8d87b82c7a2a4eabf6abbb2b7, but only lightly reviewed test. I suggested some documentation updates, but these are obviously not critical.

This change does add good test coverage and seems like an improvement over the status quo. I think I'm not really clear on the benefits of setting the blank flag in createwallet when the disable private keys flag is set, though. I wonder if we considered changing the GUI behavior to match RPC behavior instead of vice versa, and
...

💬 ryanofsky 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_r1218479148)
In commit "rpc, wallet: Set blank when disable private keys in createwallet" (5f3c64c3fd845228227e28b7cefa3e6c5c0ba7ef)

The comment is saying "We want to set BLANK when DISABLE_PRIVATE_KEYS" without saying why.

I think I would make the comment try to explain why to make the code understandable. Would suggest something like:

* By default, try to set the BLANK flag when the DISABLE_PRIVATE_KEYS flag is set, to reflect the fact that the new wallet will not contain any keys or scripts or d
...

💬 ryanofsky 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_r1218554571)
Change to unset the blank flag when writing a descriptor is now reverted, so I think this makes BLANK flag documentation misleading, and could be fixed with:

```diff
--- a/src/wallet/walletutil.h
+++ b/src/wallet/walletutil.h
@@ -53,10 +53,18 @@ enum WalletFlags : uint64_t {
//! Flag set when a wallet contains no HD seed and no private keys, scripts,
//! addresses, and other watch only things, and is therefore "blank."
//!
- //! The only function this flag serves is t
...