💬 sdaftuar commented on pull request "Draft: rework validation logic for assumeutxo":
(https://github.com/bitcoin/bitcoin/pull/27746#issuecomment-1577187405)
> How do you see the division of labor between `ChainstateManager` and `BlockManager`? The `ChainState` doc currently says:
>
> > Anything that is contingent on the current tip of the chain is stored here, whereas block information and metadata independent of the current tip is kept in `BlockManager`.

I think of `BlockManager` as being a generic backend database of block (and undo) data, while any validation-related logic should live in `Chainstate` (if it relies on chainstate-specific inf
...

💬 sdaftuar commented on pull request "Draft: rework validation logic for assumeutxo":
(https://github.com/bitcoin/bitcoin/pull/27746#issuecomment-1577188477)
> How do you see the division of labor between `ChainstateManager` and `BlockManager`? The `ChainState` doc currently says:
>
> > Anything that is contingent on the current tip of the chain is stored here, whereas block information and metadata independent of the current tip is kept in `BlockManager`.

I think of `BlockManager` as being a generic backend database of block (and undo) data, while any validation-related logic should live in `Chainstate` (if it relies on chainstate-specific inf
...

💬 pinheadmz commented on pull request "net: support unix domain sockets for -proxy and -onion":
(https://github.com/bitcoin/bitcoin/pull/27375#discussion_r1218382890)
This change would make `feature_proxy.py` test fail because the error would be `"Invalid port..."` which is misleading if the real problem is (for example) a too-long file path, which will get caught by `IsUnixSocketPath()` on L1370 and described as `"Invalid -proxy address or hostname..."`. So I think it makes more sense to keep this line as-is: if we think the user is trying to set a UNIX socket path, we just ignore the invalid port check.

💬 pinheadmz commented on pull request "net: support unix domain sockets for -proxy and -onion":
(https://github.com/bitcoin/bitcoin/pull/27375#issuecomment-1577201958)
@vasild thanks for the excellent detailed review! All comments addressed except where noted.

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