💬 l0rinc commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2546155699)
Wasn't sure whether the range of 0-5 minutes implied that the max was 14 minutes (i.e. whether the millisecond type was taken into consideration), so I ran it a few times and the plots indicate it's correct:
<img width="1008" height="618" alt="Image" src="https://github.com/user-attachments/assets/526e142c-0b77-41ab-ab3d-306a132cf7e1" />
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2546155699)
Wasn't sure whether the range of 0-5 minutes implied that the max was 14 minutes (i.e. whether the millisecond type was taken into consideration), so I ran it a few times and the plots indicate it's correct:
<img width="1008" height="618" alt="Image" src="https://github.com/user-attachments/assets/526e142c-0b77-41ab-ab3d-306a132cf7e1" />
💬 l0rinc commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2546260152)
is my understanding correct that malleated transactions can at worst leave the tx temporarily in the private broadcast queue?
nit: typo in [commit message](https://github.com/bitcoin/bitcoin/pull/29415/commits/abf841de0c504a512c559e0a056f5ca33831224f): `wxtid`
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2546260152)
is my understanding correct that malleated transactions can at worst leave the tx temporarily in the private broadcast queue?
nit: typo in [commit message](https://github.com/bitcoin/bitcoin/pull/29415/commits/abf841de0c504a512c559e0a056f5ca33831224f): `wxtid`
💬 l0rinc commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2546287626)
is my understanding correct that:
* there's no retry count limit or final timeout for retries?
* restarting the node won't continue the retries?
Can we add some unit tests for the code so that it's not only exercisably by python? Would help with debugging to understand the feature in isolation. And the multi-threaded code is a bit concerning, could we fuzz it to make sure there are no races there that are hard to reason about otherwise?
And based on the code coverage it seems to me some of
...
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2546287626)
is my understanding correct that:
* there's no retry count limit or final timeout for retries?
* restarting the node won't continue the retries?
Can we add some unit tests for the code so that it's not only exercisably by python? Would help with debugging to understand the feature in isolation. And the multi-threaded code is a bit concerning, could we fuzz it to make sure there are no races there that are hard to reason about otherwise?
And based on the code coverage it seems to me some of
...
💬 l0rinc commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2546348544)
nit: maybe we can separate the happy path from giving up via something like:
```suggestion
if (auto it_node{m_by_nodeid.find(nodeid)}; it_node != m_by_nodeid.end()) {
if (auto it_tx{m_by_txid.find(it_node->second)}; it_tx != m_by_txid.end()) {
return it_tx->second.tx;
}
}
return std::nullopt;
```
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2546348544)
nit: maybe we can separate the happy path from giving up via something like:
```suggestion
if (auto it_node{m_by_nodeid.find(nodeid)}; it_node != m_by_nodeid.end()) {
if (auto it_tx{m_by_txid.find(it_node->second)}; it_tx != m_by_txid.end()) {
return it_tx->second.tx;
}
}
return std::nullopt;
```
💬 l0rinc commented on pull request "clang-format: Set Bitcoin Core IncludeCategories":
(https://github.com/bitcoin/bitcoin/pull/33917#discussion_r2547132580)
this might as well be false, lol
(https://github.com/bitcoin/bitcoin/pull/33917#discussion_r2547132580)
this might as well be false, lol
💬 l0rinc commented on pull request "clang-format: Set Bitcoin Core IncludeCategories":
(https://github.com/bitcoin/bitcoin/pull/33917#discussion_r2547133633)
for simplicit, this can also be `false`
(https://github.com/bitcoin/bitcoin/pull/33917#discussion_r2547133633)
for simplicit, this can also be `false`
💬 l0rinc commented on pull request "clang-format: Set Bitcoin Core IncludeCategories":
(https://github.com/bitcoin/bitcoin/pull/33917#discussion_r2547130625)
nice!
How did you come up with the categories? Have you reformatted the whole codebase - or it's just experience? :)
(https://github.com/bitcoin/bitcoin/pull/33917#discussion_r2547130625)
nice!
How did you come up with the categories? Have you reformatted the whole codebase - or it's just experience? :)
💬 l0rinc commented on pull request "log: avoid collecting `GetSerializeSize` data when compact block logging is disabled":
(https://github.com/bitcoin/bitcoin/pull/33738#discussion_r2547146360)
Good idea. Should we do it in a follow-up instead?
(https://github.com/bitcoin/bitcoin/pull/33738#discussion_r2547146360)
Good idea. Should we do it in a follow-up instead?
💬 l0rinc commented on pull request "log: avoid collecting `GetSerializeSize` data when compact block logging is disabled":
(https://github.com/bitcoin/bitcoin/pull/33738#discussion_r2547148360)
I have tried explaining it in the commit message, can you please point out where more context is necessarry?
(https://github.com/bitcoin/bitcoin/pull/33738#discussion_r2547148360)
I have tried explaining it in the commit message, can you please point out where more context is necessarry?
💬 davidgumberg commented on pull request "log: avoid collecting `GetSerializeSize` data when compact block logging is disabled":
(https://github.com/bitcoin/bitcoin/pull/33738#discussion_r2547165872)
I'm not sure who else is using this logging message, but I don't think it's very helpful to print 5, in all of my branches investigating compact block reconstruction I remove this `if` statement entirely
I think we should print all of the missing transactions every time, but can definitely be discussed in a separate issue/pr.
(https://github.com/bitcoin/bitcoin/pull/33738#discussion_r2547165872)
I'm not sure who else is using this logging message, but I don't think it's very helpful to print 5, in all of my branches investigating compact block reconstruction I remove this `if` statement entirely
I think we should print all of the missing transactions every time, but can definitely be discussed in a separate issue/pr.
💬 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#issuecomment-3559469161)
CLI calls can be made to a node running a different, older version. If a researcher, data gatherer, developer, or node operator is calling this on a pre-getaddrmaninfo node, then the call should still work. As commented above, colleagues here requested that -netinfo continue to work in such cases, and we patched it. There doesn't seem to be any reason not to do the same here, rather than choosing to break it.
(https://github.com/bitcoin/bitcoin/pull/26988#issuecomment-3559469161)
CLI calls can be made to a node running a different, older version. If a researcher, data gatherer, developer, or node operator is calling this on a pre-getaddrmaninfo node, then the call should still work. As commented above, colleagues here requested that -netinfo continue to work in such cases, and we patched it. There doesn't seem to be any reason not to do the same here, rather than choosing to break it.
💬 ryanofsky commented on issue "Block template memory management (for IPC clients)":
(https://github.com/bitcoin/bitcoin/issues/33899#issuecomment-3559484781)
My takeaway from IRC meeting ([log](https://achow101.com/ircmeetings/2025/bitcoin-core-dev.2025-11-20_16_02.log.html)) is that Matt broadly wants core to be more opinionated and make more decisions on behalf of clients, while Sjors is trying to provide a more unopinionated interface. I think there is actually not much tension between these two things because we could implement everything Matt wants without changing the interface. The IRC discussion just looked to me like a debate about which opt
...
(https://github.com/bitcoin/bitcoin/issues/33899#issuecomment-3559484781)
My takeaway from IRC meeting ([log](https://achow101.com/ircmeetings/2025/bitcoin-core-dev.2025-11-20_16_02.log.html)) is that Matt broadly wants core to be more opinionated and make more decisions on behalf of clients, while Sjors is trying to provide a more unopinionated interface. I think there is actually not much tension between these two things because we could implement everything Matt wants without changing the interface. The IRC discussion just looked to me like a debate about which opt
...
💬 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_r2547219907)
I agree with preferably keeping the field names short, but the data corresponding to "addresses_known" should not change.
Perhaps use "addresses_unfiltered" or "addresses_not_filtered_for_quality_and_recency" for the new field.
As this is a CLI output, as opposed to an RPC one, its output presentation can be more optimized for human use (see -getinfo, for instance) while providing consistent historical data.
(https://github.com/bitcoin/bitcoin/pull/26988#discussion_r2547219907)
I agree with preferably keeping the field names short, but the data corresponding to "addresses_known" should not change.
Perhaps use "addresses_unfiltered" or "addresses_not_filtered_for_quality_and_recency" for the new field.
As this is a CLI output, as opposed to an RPC one, its output presentation can be more optimized for human use (see -getinfo, for instance) while providing consistent historical data.
💬 fanquake 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#issuecomment-3559517772)
> CLI calls can be made to a node running a different, older version. If a researcher, data gatherer, developer, or node operator is calling this on a pre-getaddrmaninfo node,
Could you elaborate on why these people, particularly the developers, or node operators, are running EOL versions of Core (with known CVEs); and why they can't they upgrade their nodes, but at the same time, they are downloading and using the latest version of bitcoin-cli?
> There doesn't seem to be any reason not t
...
(https://github.com/bitcoin/bitcoin/pull/26988#issuecomment-3559517772)
> CLI calls can be made to a node running a different, older version. If a researcher, data gatherer, developer, or node operator is calling this on a pre-getaddrmaninfo node,
Could you elaborate on why these people, particularly the developers, or node operators, are running EOL versions of Core (with known CVEs); and why they can't they upgrade their nodes, but at the same time, they are downloading and using the latest version of bitcoin-cli?
> There doesn't seem to be any reason not t
...
✅ Sjors closed a pull request: "mining: add requestedOutputs field, e.g. for merged mining"
(https://github.com/bitcoin/bitcoin/pull/33890)
(https://github.com/bitcoin/bitcoin/pull/33890)
💬 Sjors commented on pull request "mining: add requestedOutputs field, e.g. for merged mining":
(https://github.com/bitcoin/bitcoin/pull/33890#issuecomment-3559527326)
Going to close this because I don't think it's the right approach.
We want to keep templates as reusable as possible in order to support multiple clients (potentially even a public service, though the node would not be doing this directly).
With that in mind it seems better if clients take the template and then customize it by adding their own outputs, rather than us customising it for them.
(https://github.com/bitcoin/bitcoin/pull/33890#issuecomment-3559527326)
Going to close this because I don't think it's the right approach.
We want to keep templates as reusable as possible in order to support multiple clients (potentially even a public service, though the node would not be doing this directly).
With that in mind it seems better if clients take the template and then customize it by adding their own outputs, rather than us customising it for them.
💬 w0xlt commented on pull request "kernel, validation: Refactor ProcessNewBlock(Headers) to return BlockValidationState":
(https://github.com/bitcoin/bitcoin/pull/33856#discussion_r2547248797)
nit: Maybe the variables could be renamed in this PR for better clarity.
The “description” rows below can be converted into comments to document the variables.
| Current name | Suggestion | Description |
|----------|-------------|------|
|`state`|`accept_state`|determines if the block data is valid enough to be written to the disk and entered into the block index.|
|`bg_state`| `activation_state` |attempts to connect the block to the tip of the active chain (executing scripts and updatin
...
(https://github.com/bitcoin/bitcoin/pull/33856#discussion_r2547248797)
nit: Maybe the variables could be renamed in this PR for better clarity.
The “description” rows below can be converted into comments to document the variables.
| Current name | Suggestion | Description |
|----------|-------------|------|
|`state`|`accept_state`|determines if the block data is valid enough to be written to the disk and entered into the block index.|
|`bg_state`| `activation_state` |attempts to connect the block to the tip of the active chain (executing scripts and updatin
...
💬 maflcko commented on pull request "doc: Improve CI docs on env and qemu-user-static":
(https://github.com/bitcoin/bitcoin/pull/33887#discussion_r2547256483)
> reminds me that the `bash -c` isn't needed here, now that `env` is used. Feel free to remove, or ignore the unrelated nit.
Added a commit for this to https://github.com/bitcoin/bitcoin/pull/33903, as that CI pull didn't have any review yet anyway.
(https://github.com/bitcoin/bitcoin/pull/33887#discussion_r2547256483)
> reminds me that the `bash -c` isn't needed here, now that `env` is used. Feel free to remove, or ignore the unrelated nit.
Added a commit for this to https://github.com/bitcoin/bitcoin/pull/33903, as that CI pull didn't have any review yet anyway.
💬 hodlinator commented on pull request "doc: Improve CI docs on env and qemu-user-static":
(https://github.com/bitcoin/bitcoin/pull/33887#discussion_r2547256541)
Yes, I've seen similar segfaults when building for s390x on NixOS. Don't know if some distros apply custom patches or use other versions, or if they set up the environment differently in some other way.
(https://github.com/bitcoin/bitcoin/pull/33887#discussion_r2547256541)
Yes, I've seen similar segfaults when building for s390x on NixOS. Don't know if some distros apply custom patches or use other versions, or if they set up the environment differently in some other way.
💬 davidgumberg commented on pull request "log: avoid collecting `GetSerializeSize` data when compact block logging is disabled":
(https://github.com/bitcoin/bitcoin/pull/33738#discussion_r2547272545)
> Is this guaranteed to iterate over all members of vtx_missing[]?
Yes, because of the check below that:
https://github.com/bitcoin/bitcoin/blob/6b2d17b13220ea69c33b8cab41fe059ff758874c/src/blockencodings.cpp#L211-L212
We increment `tx_missing_offset` every time we take a transaction from `vtx_missing`, so the check enforces that we followed the `if` branch / took from `vtx_missing` just as many times as there are members in `vtx_missing`.
(https://github.com/bitcoin/bitcoin/pull/33738#discussion_r2547272545)
> Is this guaranteed to iterate over all members of vtx_missing[]?
Yes, because of the check below that:
https://github.com/bitcoin/bitcoin/blob/6b2d17b13220ea69c33b8cab41fe059ff758874c/src/blockencodings.cpp#L211-L212
We increment `tx_missing_offset` every time we take a transaction from `vtx_missing`, so the check enforces that we followed the `if` branch / took from `vtx_missing` just as many times as there are members in `vtx_missing`.