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

💬 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_r1218522240)
In commit "tests: Test that the blank wallet flag is un/set as expected" (7baa0971527ba3c8d87b82c7a2a4eabf6abbb2b7)

Would be nice to add the test earlier as the second commit or part of the first commit, so it would be easily possible to see how other commits in the PR affect the test cases