💬 instagibbs commented on pull request "Relay own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/27509#issuecomment-1577106769)
> I think if sensitive relay is used this can be made more aggressive and/or increase the number of peers we broadcast to. E.g. after 3 minutes broadcast to 10 peers.
I think for first cut matching current behavior is probably best. If Tor is being sybiled, I don't think hammering things even harder is going to fix much. Time-sensitive users should probably just avoid using this feature altogether, at least until there is some sort of fallback mechanism introduced.
(https://github.com/bitcoin/bitcoin/pull/27509#issuecomment-1577106769)
> I think if sensitive relay is used this can be made more aggressive and/or increase the number of peers we broadcast to. E.g. after 3 minutes broadcast to 10 peers.
I think for first cut matching current behavior is probably best. If Tor is being sybiled, I don't think hammering things even harder is going to fix much. Time-sensitive users should probably just avoid using this feature altogether, at least until there is some sort of fallback mechanism introduced.
💬 miketwenty1 commented on pull request "rpc: fix data optionality for RPC calls.":
(https://github.com/bitcoin/bitcoin/pull/27829#issuecomment-1577132445)
> I get that this is confusing, but this change isn't correct.
>
> The `"data"` field itself is required when the object is present (as in `[{"address": amount, ..., {}]` would not be valid). What's optional is the object (the `{ ... }` around it), which is at a higher level.
>
> Maybe it's worth elaborating in the prose text above?
Understood you're saying the change isn't correct. How would someone properly indicate the "data" is optional if not specifying it as optional? The parent o
...
(https://github.com/bitcoin/bitcoin/pull/27829#issuecomment-1577132445)
> I get that this is confusing, but this change isn't correct.
>
> The `"data"` field itself is required when the object is present (as in `[{"address": amount, ..., {}]` would not be valid). What's optional is the object (the `{ ... }` around it), which is at a higher level.
>
> Maybe it's worth elaborating in the prose text above?
Understood you're saying the change isn't correct. How would someone properly indicate the "data" is optional if not specifying it as optional? The parent o
...
💬 sipa commented on pull request "rpc: fix data optionality for RPC calls.":
(https://github.com/bitcoin/bitcoin/pull/27829#issuecomment-1577142430)
The `"data": ...` part cannot be left out. It's the `{"data": ...}` that can be left out.
(https://github.com/bitcoin/bitcoin/pull/27829#issuecomment-1577142430)
The `"data": ...` part cannot be left out. It's the `{"data": ...}` that can be left out.
💬 miketwenty1 commented on pull request "rpc: fix data optionality for RPC calls.":
(https://github.com/bitcoin/bitcoin/pull/27829#issuecomment-1577147866)
> The `"data": ...` part cannot be left out. It's the `{"data": ...}` that can be left out.
Let me rephrase my question:
How would someone know `{"data" ...}` is optional if not specifying it as optional? If every field is optional within an obj maybe it's implied the obj itself is optional?
Does there need to be a new idea of specifying whether the (json object) itself is optional?
(https://github.com/bitcoin/bitcoin/pull/27829#issuecomment-1577147866)
> The `"data": ...` part cannot be left out. It's the `{"data": ...}` that can be left out.
Let me rephrase my question:
How would someone know `{"data" ...}` is optional if not specifying it as optional? If every field is optional within an obj maybe it's implied the obj itself is optional?
Does there need to be a new idea of specifying whether the (json object) itself is optional?
💬 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.
(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
(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
...
(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
...
(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
...
(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.
(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.
(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
...
(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`.
(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
(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<sup>64</sup>
2. Adding two `uint64_t` values is performed mod 2<sup>64</sup>
Also, the new code is equivalent to previous code beca
...
(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<sup>64</sup>
2. Adding two `uint64_t` values is performed mod 2<sup>64</sup>
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.
(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
(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)
(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
(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
...
(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. 🤞
(https://github.com/bitcoin/bitcoin/pull/27557#issuecomment-1577271267)
> Needs rebase?
ty. 🤞