💬 Sjors commented on pull request "Add waitFeesChanged() to Mining interface":
(https://github.com/bitcoin/bitcoin/pull/31003#discussion_r1840192430)
#31283 returns a block template

💬 Sjors commented on pull request "Add waitFeesChanged() to Mining interface":
(https://github.com/bitcoin/bitcoin/pull/31003#discussion_r1840192936)
#31283 returns a block template

💬 fanquake commented on pull request "guix: scope pkg-config to Linux only":
(https://github.com/bitcoin/bitcoin/pull/31276#issuecomment-2473494323)
Guix build (aarch64)
```bash
a9e1f628dd64913209e6f55f183453b046c91e44fb959ea61e0c40c0a8457e98 guix-build-2833befb7822/output/aarch64-linux-gnu/SHA256SUMS.part
d082fbc96abb72a4bb0495833b6361d1ab61f8eefc3423571589920ed6c89678 guix-build-2833befb7822/output/aarch64-linux-gnu/bitcoin-2833befb7822-aarch64-linux-gnu-debug.tar.gz
6d94622d87e86ceef908e2b05435ac7f0e4b7f3d224454f619155e44c36128b9 guix-build-2833befb7822/output/aarch64-linux-gnu/bitcoin-2833befb7822-aarch64-linux-gnu.tar.gz
ca28805
...

💬 Sjors commented on pull request "Add waitNext() to BlockTemplate interface":
(https://github.com/bitcoin/bitcoin/pull/31283#discussion_r1840195105)
I still need to study https://github.com/bitcoin/bitcoin/pull/31003#discussion_r1792221843 in more detail to see if this code isn't making similar mistakes.

🤔 BrandonOdiwuor reviewed a pull request: "build: Switch to Qt 6"
(https://github.com/bitcoin/bitcoin/pull/30997#pullrequestreview-2433034814)
Concept ACK

💬 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.

💬 sipa commented on pull request "validation: Remove RECENT_CONSENSUS_CHANGE validation result":
(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.

💬 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
```

💬 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?

💬 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
...

💬 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

💬 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
...

💬 polespinasa commented on pull request "wallet, rpc: Settxfeerate":
(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.

💬 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

💬 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?

👍 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`

💬 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.

💬 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.