💬 mzumsande commented on pull request "p2p: Restrict self-advertisements with privacy networks to avoid fingerprinting":
(https://github.com/bitcoin/bitcoin/pull/27411#issuecomment-1577161538)
> Needs rebase?

yes. Rebased due to silent conflict with #26261.

💬 pinheadmz commented on pull request "Relay own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/27509#issuecomment-1577177916)
> Tor-to-IPv4 exit nodes,

I was more asking about this, not as much from a vulnerability standpoint but more like, the bottleneck created by all TXs going through a specific subset of network nodes first. I think your envelope math is reassuring though - I didn't realize there were so many Tor nodes already

💬 mzumsande commented on pull request "init: return error when block index is non-contiguous, fix feature_init.py file perturbation":
(https://github.com/bitcoin/bitcoin/pull/27823#discussion_r1218371741)
> Wouldn't that be broken if we ever add a parallel headers download process?
> Which IIRC we don't have because they are downloaded from a single peer at time. But if we implement it, we could get orphan block indexes pretty easily.

Yes, but only we had a very naive parallel headers download process. I would imagine that if we ever parallelized headers download, we'd take care not to accept orphaned block indexes into our main block index (for DoS reasons) but would keep those headers in so
...

💬 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