🤔 glozow reviewed a pull request: "node: change a tx-relay on/off flag to enum"
(https://github.com/bitcoin/bitcoin/pull/33567#pullrequestreview-3405305256)
utACK 07a926474b5a6fa1d3d4656362a0117611f6da2f
(https://github.com/bitcoin/bitcoin/pull/33567#pullrequestreview-3405305256)
utACK 07a926474b5a6fa1d3d4656362a0117611f6da2f
💬 glozow commented on pull request "node: change a tx-relay on/off flag to enum":
(https://github.com/bitcoin/bitcoin/pull/33567#discussion_r2482173040)
nit: "what" is a pretty unhelpful variable name
(https://github.com/bitcoin/bitcoin/pull/33567#discussion_r2482173040)
nit: "what" is a pretty unhelpful variable name
💬 glozow commented on pull request "node: change a tx-relay on/off flag to enum":
(https://github.com/bitcoin/bitcoin/pull/33567#discussion_r2482168470)
Awkward part about splitting this off: adding it to mempool is not actually optional at this point in time.
(https://github.com/bitcoin/bitcoin/pull/33567#discussion_r2482168470)
Awkward part about splitting this off: adding it to mempool is not actually optional at this point in time.
🚀 glozow merged a pull request: "node: change a tx-relay on/off flag to enum"
(https://github.com/bitcoin/bitcoin/pull/33567)
(https://github.com/bitcoin/bitcoin/pull/33567)
💬 ajtowns commented on pull request "miner: don't needlessly append dummy OP_0 to bip34":
(https://github.com/bitcoin/bitcoin/pull/32420#issuecomment-3474555715)
> I also don't think `CBlockTemplate` should have an `extraNonce`.
I think a **block** should (almost) always have an extraNonce -- whether for mainnet to provide sufficient work, or for early blocks on regtest/signet to avoid bad-cb-length, or for test net blocks to more closely simulate mainnet behaviour. Whether the template should also include an extraNonce depends on the user of the template -- if they want a template they can just apply work to via nNonce, without further fussing about
...
(https://github.com/bitcoin/bitcoin/pull/32420#issuecomment-3474555715)
> I also don't think `CBlockTemplate` should have an `extraNonce`.
I think a **block** should (almost) always have an extraNonce -- whether for mainnet to provide sufficient work, or for early blocks on regtest/signet to avoid bad-cb-length, or for test net blocks to more closely simulate mainnet behaviour. Whether the template should also include an extraNonce depends on the user of the template -- if they want a template they can just apply work to via nNonce, without further fussing about
...
💬 glozow commented on issue "test: break `feature_block` into subtests?":
(https://github.com/bitcoin/bitcoin/issues/32877#issuecomment-3474586758)
Have we considered giving functional tests severity ratings, like with bench? Some tests take longer because they cover things that are harder to reach. We also have some things that go on timers and it would be nice to not mock the times all the time, but it's annoying if they are bottlenecks.
(https://github.com/bitcoin/bitcoin/issues/32877#issuecomment-3474586758)
Have we considered giving functional tests severity ratings, like with bench? Some tests take longer because they cover things that are harder to reach. We also have some things that go on timers and it would be nice to not mock the times all the time, but it's annoying if they are bottlenecks.
💬 ajtowns commented on pull request "Relax standardness rules regarding CHECKMULTISIG":
(https://github.com/bitcoin/bitcoin/pull/33755#issuecomment-3474594418)
If you're deliberately changing the standardness rules specified in BIP143, that should be in the PR description, and possibly proposed as an update to the BIP. If you're not deliberately changing them, they shouldn't be changed by accident. Personally, I don't see the justification for changing them here, though I don't really understand why it was specced as a policy rule rather than a consensus one at the time, either.
Personally, I don't really understand the motivation for reconsidering
...
(https://github.com/bitcoin/bitcoin/pull/33755#issuecomment-3474594418)
If you're deliberately changing the standardness rules specified in BIP143, that should be in the PR description, and possibly proposed as an update to the BIP. If you're not deliberately changing them, they shouldn't be changed by accident. Personally, I don't see the justification for changing them here, though I don't really understand why it was specced as a policy rule rather than a consensus one at the time, either.
Personally, I don't really understand the motivation for reconsidering
...
💬 roconnor-blockstream commented on pull request "Relax standardness rules regarding CHECKMULTISIG":
(https://github.com/bitcoin/bitcoin/pull/33755#issuecomment-3474639402)
I guess we could change BIP143. Currently the text reads:
> Each public key passed to a sigop inside version 0 witness program must be a compressed key: the first byte MUST be either 0x02 or 0x03, and the size MUST be 33 bytes. Transactions that break this rule will not be relayed or mined by default.
Note that is not how Bitcoin Core implements `SCRIPT_VERIFY_WITNESS_PUBKEYTYPE` currently as you can pass pubkeys to CHECKMULTISIG that violate this policy when the signature stack is used
...
(https://github.com/bitcoin/bitcoin/pull/33755#issuecomment-3474639402)
I guess we could change BIP143. Currently the text reads:
> Each public key passed to a sigop inside version 0 witness program must be a compressed key: the first byte MUST be either 0x02 or 0x03, and the size MUST be 33 bytes. Transactions that break this rule will not be relayed or mined by default.
Note that is not how Bitcoin Core implements `SCRIPT_VERIFY_WITNESS_PUBKEYTYPE` currently as you can pass pubkeys to CHECKMULTISIG that violate this policy when the signature stack is used
...
💬 da1sychain commented on pull request "doc: document fingerprinting risk when operating node on multiple networks":
(https://github.com/bitcoin/bitcoin/pull/33750#discussion_r2482509366)
Thank you, good idea. Added some clarifying language to that sentence.
(https://github.com/bitcoin/bitcoin/pull/33750#discussion_r2482509366)
Thank you, good idea. Added some clarifying language to that sentence.
💬 darosior commented on pull request "doc: document fingerprinting risk when operating node on multiple networks":
(https://github.com/bitcoin/bitcoin/pull/33750#issuecomment-3474716867)
> See #33498.
This is only one such vector. It's good to have a warning until we can be reasonably confident that it's not trivially possible to link two different network addresses of a node. I am also skeptical we could ever reach this level of confidence.
(https://github.com/bitcoin/bitcoin/pull/33750#issuecomment-3474716867)
> See #33498.
This is only one such vector. It's good to have a warning until we can be reasonably confident that it's not trivially possible to link two different network addresses of a node. I am also skeptical we could ever reach this level of confidence.
💬 roconnor-blockstream commented on pull request "Relax standardness rules regarding CHECKMULTISIG":
(https://github.com/bitcoin/bitcoin/pull/33755#issuecomment-3474717031)
> If there were real coins locked due to this policy, it seems like they should have been already recovered via getting a non-standard spend mined directly.
I don't know why you would think that they should have already been recovered. You think anyone can just call up a mining pool and get their non-standard transactions mined? How much money needs to be at stake before you can even get a pool's attention? Or how small in value do UTXOs have to be so that it is okay to soft-confiscate it
...
(https://github.com/bitcoin/bitcoin/pull/33755#issuecomment-3474717031)
> If there were real coins locked due to this policy, it seems like they should have been already recovered via getting a non-standard spend mined directly.
I don't know why you would think that they should have already been recovered. You think anyone can just call up a mining pool and get their non-standard transactions mined? How much money needs to be at stake before you can even get a pool's attention? Or how small in value do UTXOs have to be so that it is okay to soft-confiscate it
...
🤔 janb84 reviewed a pull request: "refactor: unify container presence checks"
(https://github.com/bitcoin/bitcoin/pull/33192#pullrequestreview-3405953584)
re ACK 21cff12b0e6dc58fb8c66543bbd713796d9941ae
I still think this a good idea, it may be a "style change" but the increased clarity will decrease bugs and mental load when reading the code.
changes since last ack:
- reduced changes to not hit cluster mempool.
(https://github.com/bitcoin/bitcoin/pull/33192#pullrequestreview-3405953584)
re ACK 21cff12b0e6dc58fb8c66543bbd713796d9941ae
I still think this a good idea, it may be a "style change" but the increased clarity will decrease bugs and mental load when reading the code.
changes since last ack:
- reduced changes to not hit cluster mempool.
🤔 jonatack reviewed a pull request: "doc: document fingerprinting risk when operating node on multiple networks"
(https://github.com/bitcoin/bitcoin/pull/33750#pullrequestreview-3406173039)
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/33750#pullrequestreview-3406173039)
Concept ACK
💬 jonatack commented on pull request "doc: document fingerprinting risk when operating node on multiple networks":
(https://github.com/bitcoin/bitcoin/pull/33750#discussion_r2482681588)
Idem.
```suggestion
- Operating a node that listens on multiple networks (e.g., Tor and I2P) can help
```
(https://github.com/bitcoin/bitcoin/pull/33750#discussion_r2482681588)
Idem.
```suggestion
- Operating a node that listens on multiple networks (e.g., Tor and I2P) can help
```
💬 jonatack commented on pull request "doc: document fingerprinting risk when operating node on multiple networks":
(https://github.com/bitcoin/bitcoin/pull/33750#discussion_r2482683578)
While the study linked to in the OP described IPv4 and Tor, I suspect the most frequent multiple network pairing would be Tor and I2P, as privacy network users might opt to avoid clearnet (say, the kind of node operator that pre-I2P may have been running over Tor only).
Suggested change
- Operating a node that listens on multiple networks (e.g. IPv4 and I2P) can help
...
(https://github.com/bitcoin/bitcoin/pull/33750#discussion_r2482683578)
While the study linked to in the OP described IPv4 and Tor, I suspect the most frequent multiple network pairing would be Tor and I2P, as privacy network users might opt to avoid clearnet (say, the kind of node operator that pre-I2P may have been running over Tor only).
Suggested change
- Operating a node that listens on multiple networks (e.g. IPv4 and I2P) can help
...
💬 polespinasa commented on pull request "rpc: Optionally print feerates in sat/vb":
(https://github.com/bitcoin/bitcoin/pull/33741#discussion_r2482777067)
> Generally, it is best to write unit or fuzz tests for newly written code
Done
> Also, I am not sure if `CFeeRate` should be bundled with univalue. The conversion could be a standalone helper, but this is just a nit.
I decided to make it a function of `CFeeRate` because is the only class that should use it. But I'm ok moving it to `core_writte`.
(https://github.com/bitcoin/bitcoin/pull/33741#discussion_r2482777067)
> Generally, it is best to write unit or fuzz tests for newly written code
Done
> Also, I am not sure if `CFeeRate` should be bundled with univalue. The conversion could be a standalone helper, but this is just a nit.
I decided to make it a function of `CFeeRate` because is the only class that should use it. But I'm ok moving it to `core_writte`.
🤔 jonatack reviewed a pull request: "cli: rework -addrinfo cli to use addresses which aren’t filtered for quality/recency"
(https://github.com/bitcoin/bitcoin/pull/26988#pullrequestreview-3406254553)
Approach ACK 016ab85a13408ad980c3dbce4e041e14a4fcf3b8, thanks for updating, tested with a server on master, not yet tested with a server running pre-v26 (before getaddrmaninfo)
```
$ ./build/bin/bitcoin-cli -addrinfo
{
"all addresses known (used for selecting peers)": {
"ipv4": 48133,
"ipv6": 8017,
"onion": 17257,
"i2p": 3757,
"cjdns": 11,
"total": 77175
},
"addresses known after quality/recency filtering (for original -addrinfo compatibility)": {
"ipv4": 45921
...
(https://github.com/bitcoin/bitcoin/pull/26988#pullrequestreview-3406254553)
Approach ACK 016ab85a13408ad980c3dbce4e041e14a4fcf3b8, thanks for updating, tested with a server on master, not yet tested with a server running pre-v26 (before getaddrmaninfo)
```
$ ./build/bin/bitcoin-cli -addrinfo
{
"all addresses known (used for selecting peers)": {
"ipv4": 48133,
"ipv6": 8017,
"onion": 17257,
"i2p": 3757,
"cjdns": 11,
"total": 77175
},
"addresses known after quality/recency filtering (for original -addrinfo compatibility)": {
"ipv4": 45921
...
💬 jonatack commented on pull request "cli: rework -addrinfo cli to use addresses which aren’t filtered for quality/recency":
(https://github.com/bitcoin/bitcoin/pull/26988#discussion_r2482731134)
```suggestion
- CLI -addrinfo previously (v22 - v30) returned addresses known to the node after filtering for quality and recency.
```
(https://github.com/bitcoin/bitcoin/pull/26988#discussion_r2482731134)
```suggestion
- CLI -addrinfo previously (v22 - v30) returned addresses known to the node after filtering for quality and recency.
```
💬 jonatack commented on pull request "cli: rework -addrinfo cli to use addresses which aren’t filtered for quality/recency":
(https://github.com/bitcoin/bitcoin/pull/26988#discussion_r2482783485)
```suggestion
* - getnodeaddresses (v22.0+) for stats of addresses in the addrman filtered by quality (i.e. not IsTerrible), used by -addrinfo since v22.0 and kept for data continuity)
```
(https://github.com/bitcoin/bitcoin/pull/26988#discussion_r2482783485)
```suggestion
* - getnodeaddresses (v22.0+) for stats of addresses in the addrman filtered by quality (i.e. not IsTerrible), used by -addrinfo since v22.0 and kept for data continuity)
```
💬 da1sychain commented on pull request "doc: document fingerprinting risk when operating node on multiple networks":
(https://github.com/bitcoin/bitcoin/pull/33750#discussion_r2482805132)
This combination, IMO, poses less risk to the privacy-concerned individual as they are exposing their node over two privacy networks. In fact I think the risk in this configuration would be lean more towards isolation / partition.
(https://github.com/bitcoin/bitcoin/pull/33750#discussion_r2482805132)
This combination, IMO, poses less risk to the privacy-concerned individual as they are exposing their node over two privacy networks. In fact I think the risk in this configuration would be lean more towards isolation / partition.