💬 hodlinator commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1840257211)
Not sure what you mean, seems you are arguing in the reverse direction or talking past?
I'm thinking the main use-case is for a miner or pool to increase priority of a transaction due to them getting paid out-of-band. While that is bad from a centralization aspect, having such a facility in unpatched versions of Bitcoin Core means miners/pools have an easier time keeping their nodes up-to-date (less moving parts), meaning they are more responsive to our releases.
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1840257211)
Not sure what you mean, seems you are arguing in the reverse direction or talking past?
I'm thinking the main use-case is for a miner or pool to increase priority of a transaction due to them getting paid out-of-band. While that is bad from a centralization aspect, having such a facility in unpatched versions of Bitcoin Core means miners/pools have an easier time keeping their nodes up-to-date (less moving parts), meaning they are more responsive to our releases.
💬 sipa commented on pull request "validation: Remove RECENT_CONSENSUS_CHANGE validation result":
(https://github.com/bitcoin/bitcoin/pull/31269#issuecomment-2473610234)
ACK e80e4c6ff91e27d7d40f099a2d7942c29085234c
(https://github.com/bitcoin/bitcoin/pull/31269#issuecomment-2473610234)
ACK e80e4c6ff91e27d7d40f099a2d7942c29085234c
💬 hodlinator commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1840280274)
Ah, totally missed that. Cheers!
Not sure what they add, it's not like there other transactions in the way. I guess they test that nothing funky is going on with the prioritization-logic resulting in dusty transactions being accepted when they shouldn't be.
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1840280274)
Ah, totally missed that. Cheers!
Not sure what they add, it's not like there other transactions in the way. I guess they test that nothing funky is going on with the prioritization-logic resulting in dusty transactions being accepted when they shouldn't be.
💬 TheCharlatan commented on pull request "refactor: Make node_id a const& in RemoveBlockRequest":
(https://github.com/bitcoin/bitcoin/pull/31282#issuecomment-2473659364)
Looks like iwyu is still complaining about some of the includes:
```
[08:23:24.120] (/ci_container_base/src/net_processing.h has correct #includes/fwd-decls)
[08:23:24.120]
[08:23:24.120] /ci_container_base/src/net_processing.cpp should add these lines:
[08:23:24.120]
[08:23:24.120] /ci_container_base/src/net_processing.cpp should remove these lines:
[08:23:24.120] - #include <deploymentstatus.h> // lines 21-21
[08:23:24.120] - #include <node/warnings.h> // lines 39-39
```
(https://github.com/bitcoin/bitcoin/pull/31282#issuecomment-2473659364)
Looks like iwyu is still complaining about some of the includes:
```
[08:23:24.120] (/ci_container_base/src/net_processing.h has correct #includes/fwd-decls)
[08:23:24.120]
[08:23:24.120] /ci_container_base/src/net_processing.cpp should add these lines:
[08:23:24.120]
[08:23:24.120] /ci_container_base/src/net_processing.cpp should remove these lines:
[08:23:24.120] - #include <deploymentstatus.h> // lines 21-21
[08:23:24.120] - #include <node/warnings.h> // lines 39-39
```
💬 maflcko commented on pull request "refactor: Make node_id a const& in RemoveBlockRequest":
(https://github.com/bitcoin/bitcoin/pull/31282#issuecomment-2473688687)
> Looks like iwyu is still complaining about some of the includes:
That's an upstream bug. Maybe a separate issue can be used to track those?
(https://github.com/bitcoin/bitcoin/pull/31282#issuecomment-2473688687)
> Looks like iwyu is still complaining about some of the includes:
That's an upstream bug. Maybe a separate issue can be used to track those?
💬 instagibbs commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1840334098)
> Not sure what you mean, seems you are arguing in the reverse direction or talking past?
There are two places where we disallow modified fees to take effect:
1) prioritisetransaction, when the transaction is already in the mempool
2) on entry to mempool, in PreChecks
This ensures the invariant that unspent dust doesn't enter the utxo set ever (modulo reducing minrelay and block mining fee to 0).
We're assuming node operators/miners care about dust. If they do not, they can already
...
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1840334098)
> Not sure what you mean, seems you are arguing in the reverse direction or talking past?
There are two places where we disallow modified fees to take effect:
1) prioritisetransaction, when the transaction is already in the mempool
2) on entry to mempool, in PreChecks
This ensures the invariant that unspent dust doesn't enter the utxo set ever (modulo reducing minrelay and block mining fee to 0).
We're assuming node operators/miners care about dust. If they do not, they can already
...
💬 polespinasa commented on pull request "wallet, rpc: Settxfeerate":
(https://github.com/bitcoin/bitcoin/pull/31278#discussion_r1840339524)
I will mark this as resolved as we're discussing the same in the other comment thread
(https://github.com/bitcoin/bitcoin/pull/31278#discussion_r1840339524)
I will mark this as resolved as we're discussing the same in the other comment thread
💬 0xB10C commented on pull request "tracing: network connection tracepoints":
(https://github.com/bitcoin/bitcoin/pull/25832#discussion_r1840345151)
> What would it do if the argument is longer than the supplied buffer? I presume the last arg is the length of the buffer, so it is not going to overflow, I guess.
Correct, it would cut it off.
> But would it add a trailing '\0'?
No. However there is [bpf_probe_read_user_str()](https://docs.ebpf.io/linux/helper-function/bpf_probe_read_user_str/) which does that. This generally seems to be the better suited methodology here. I'll use that for the changes in this PR. There are other plac
...
(https://github.com/bitcoin/bitcoin/pull/25832#discussion_r1840345151)
> What would it do if the argument is longer than the supplied buffer? I presume the last arg is the length of the buffer, so it is not going to overflow, I guess.
Correct, it would cut it off.
> But would it add a trailing '\0'?
No. However there is [bpf_probe_read_user_str()](https://docs.ebpf.io/linux/helper-function/bpf_probe_read_user_str/) which does that. This generally seems to be the better suited methodology here. I'll use that for the changes in this PR. There are other plac
...
💬 polespinasa commented on pull request "wallet, rpc: Settxfeerate":
(https://github.com/bitcoin/bitcoin/pull/31278#discussion_r1840346467)
Thanks!
Applied following some of the examples.
(https://github.com/bitcoin/bitcoin/pull/31278#discussion_r1840346467)
Thanks!
Applied following some of the examples.
💬 0xB10C commented on pull request "tracing: network connection tracepoints":
(https://github.com/bitcoin/bitcoin/pull/25832#discussion_r1840354778)
Correct, this is an oversight from an older version of this PR. Likely related to https://github.com/bitcoin/bitcoin/pull/25832#discussion_r1692884230.
(https://github.com/bitcoin/bitcoin/pull/25832#discussion_r1840354778)
Correct, this is an oversight from an older version of this PR. Likely related to https://github.com/bitcoin/bitcoin/pull/25832#discussion_r1692884230.
💬 instagibbs commented on pull request "Package validation: accept packages of size 1":
(https://github.com/bitcoin/bitcoin/pull/31096#discussion_r1840359897)
Left a comment, I think my one-liner is the cleanest correct solution?
pushed it as a separate commit
(https://github.com/bitcoin/bitcoin/pull/31096#discussion_r1840359897)
Left a comment, I think my one-liner is the cleanest correct solution?
pushed it as a separate commit
💬 andrewtoth commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r1840396964)
But we are adding a new option here `privatebroadcast`. Could we not change the semantics of `connect` *if* the new option `privatebroadcast` is used?
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r1840396964)
But we are adding a new option here `privatebroadcast`. Could we not change the semantics of `connect` *if* the new option `privatebroadcast` is used?
👍 instagibbs approved a pull request: "cluster mempool: Implement changeset interface for mempool"
(https://github.com/bitcoin/bitcoin/pull/31122#pullrequestreview-2433424943)
ACK 0ca1e05d8bcbc42892156c15f128a5f829e9e48e
primary change is the tracing commit, which seems to make sense though I am not an expert
via `git range-diff master 9512bd34a3d74d15c3ce1086a80a1edfa5ac3acf 0ca1e05d8bcbc42892156c15f128a5f829e9e48e`
(https://github.com/bitcoin/bitcoin/pull/31122#pullrequestreview-2433424943)
ACK 0ca1e05d8bcbc42892156c15f128a5f829e9e48e
primary change is the tracing commit, which seems to make sense though I am not an expert
via `git range-diff master 9512bd34a3d74d15c3ce1086a80a1edfa5ac3acf 0ca1e05d8bcbc42892156c15f128a5f829e9e48e`
💬 0xB10C commented on pull request "tracing: network connection tracepoints":
(https://github.com/bitcoin/bitcoin/pull/25832#discussion_r1840447218)
Good catch. I confirmed that the `a`'s actually show up in the tracing scripts and are cut off. I think it's fine to leave 68 in the tests, but I'll add a footnote in the docs in a separate commit.
(https://github.com/bitcoin/bitcoin/pull/25832#discussion_r1840447218)
Good catch. I confirmed that the `a`'s actually show up in the tracing scripts and are cut off. I think it's fine to leave 68 in the tests, but I'll add a footnote in the docs in a separate commit.
💬 vasild commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r1840474930)
Yeah, you are right that `-connect=0 -privatebroadcast=1` did not exist before because `-privatebroadcast=` did not exist. Then the semantic of `-connect=0` would be "do this if privatebroadcast=0 and do something else if privatebroadcast=1". I do not like that, seems convoluted and a possible source of confusion to make the behavior of one option dependent on the value of another option. I would like to have less of that stuff, not more.
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r1840474930)
Yeah, you are right that `-connect=0 -privatebroadcast=1` did not exist before because `-privatebroadcast=` did not exist. Then the semantic of `-connect=0` would be "do this if privatebroadcast=0 and do something else if privatebroadcast=1". I do not like that, seems convoluted and a possible source of confusion to make the behavior of one option dependent on the value of another option. I would like to have less of that stuff, not more.
💬 fanquake commented on issue "TSAN/MSAN fails with vm.mmap_rnd_bits=32 even with llvm 18.1.3":
(https://github.com/bitcoin/bitcoin/issues/30674#issuecomment-2473835423)
Has started happening again in the TSAN CI (Clang 19.1.4)? https://cirrus-ci.com/task/5338932714405888?logs=ci#L2213
(https://github.com/bitcoin/bitcoin/issues/30674#issuecomment-2473835423)
Has started happening again in the TSAN CI (Clang 19.1.4)? https://cirrus-ci.com/task/5338932714405888?logs=ci#L2213
💬 instagibbs commented on pull request "validation: Remove RECENT_CONSENSUS_CHANGE validation result":
(https://github.com/bitcoin/bitcoin/pull/31269#issuecomment-2473840395)
@sdaftuar since you're the one who introduced this flag IIRC, mind weighing in?
(https://github.com/bitcoin/bitcoin/pull/31269#issuecomment-2473840395)
@sdaftuar since you're the one who introduced this flag IIRC, mind weighing in?
💬 0xB10C commented on pull request "tracing: network connection tracepoints":
(https://github.com/bitcoin/bitcoin/pull/25832#discussion_r1840483719)
I think this can be handled on the user side. The bpftrace tool cuts of strings after 64 chars by default and bcc and libbpf both have [bpf_probe_read_user_str()](https://docs.ebpf.io/linux/helper-function/bpf_probe_read_user_str/), which would replace the last char with `\0` on overflow.
I haven't had any problems with this using the tracepoints in Python and Rust. I did a quick check with a C program, and using `bpf_probe_read_user_str()` seems fine: https://github.com/0xB10C/libbpf-bootst
...
(https://github.com/bitcoin/bitcoin/pull/25832#discussion_r1840483719)
I think this can be handled on the user side. The bpftrace tool cuts of strings after 64 chars by default and bcc and libbpf both have [bpf_probe_read_user_str()](https://docs.ebpf.io/linux/helper-function/bpf_probe_read_user_str/), which would replace the last char with `\0` on overflow.
I haven't had any problems with this using the tracepoints in Python and Rust. I did a quick check with a C program, and using `bpf_probe_read_user_str()` seems fine: https://github.com/0xB10C/libbpf-bootst
...
💬 polespinasa commented on pull request "wallet, rpc: Settxfeerate":
(https://github.com/bitcoin/bitcoin/pull/31278#issuecomment-2473845095)
@fjahr @ismaelsadeeq ``txfeerate`` is now deprecated, but I kept it hidden. IMO it makes sense to have a deprecated rpc call hidden as only users who were using it before should know about it and may need info, they can continue using ``help settxfee``
(https://github.com/bitcoin/bitcoin/pull/31278#issuecomment-2473845095)
@fjahr @ismaelsadeeq ``txfeerate`` is now deprecated, but I kept it hidden. IMO it makes sense to have a deprecated rpc call hidden as only users who were using it before should know about it and may need info, they can continue using ``help settxfee``
💬 andrewtoth commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r1840503929)
I suppose I can `addnode` my trusted node for now, but I will be leaking other outbound connections.
I'm not sure how to handle my case unless we don't consider the ephemeral broadcast connections as true "connections" in the sense of `connect` option.
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r1840503929)
I suppose I can `addnode` my trusted node for now, but I will be leaking other outbound connections.
I'm not sure how to handle my case unless we don't consider the ephemeral broadcast connections as true "connections" in the sense of `connect` option.