💬 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.
💬 andrewtoth commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r1840504627)
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_r1840504627)
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.
💬 m3dwards commented on pull request "ci: skip Github CI on branch pushes for forks":
(https://github.com/bitcoin/bitcoin/pull/30487#issuecomment-2473926492)
ACK 8610bcef9d030013f9e36cffe0c58dd2cfe85d66
(https://github.com/bitcoin/bitcoin/pull/30487#issuecomment-2473926492)
ACK 8610bcef9d030013f9e36cffe0c58dd2cfe85d66
💬 l0rinc commented on pull request "optimization: change XOR obfuscation key from `std::vector<std::byte>{8}` to `uint64_t`":
(https://github.com/bitcoin/bitcoin/pull/31144#issuecomment-2473947449)
> I understand you were burned by endianness but I disagree that it's worth sacrificing readability where endianness is a non-issue.
Thanks @hodlinator for the suggestions, I tried them all, but in the end decided that I value consistency more than coming up with a separate solution for each test case. These are ugly, I agree, but at least they're testing the setup we're using in prod.
I did however change the hard-coded 8 values to `sizeof xor_key` (for memcpy) or `sizeof(uint64_t)` for vec
...
(https://github.com/bitcoin/bitcoin/pull/31144#issuecomment-2473947449)
> I understand you were burned by endianness but I disagree that it's worth sacrificing readability where endianness is a non-issue.
Thanks @hodlinator for the suggestions, I tried them all, but in the end decided that I value consistency more than coming up with a separate solution for each test case. These are ugly, I agree, but at least they're testing the setup we're using in prod.
I did however change the hard-coded 8 values to `sizeof xor_key` (for memcpy) or `sizeof(uint64_t)` for vec
...