💬 yuvicc commented on pull request "validation: Remove min_pow_checked arg in ProcessNewBlockHeaders":
(https://github.com/bitcoin/bitcoin/pull/34022#issuecomment-3621481762)
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/34022#issuecomment-3621481762)
Concept ACK
💬 Ataraxia009 commented on pull request "log: don't rate-limit "new peer" with -debug=net":
(https://github.com/bitcoin/bitcoin/pull/34008#issuecomment-3621582615)
Personally I prefer this approach over https://github.com/bitcoin/bitcoin/pull/34018 @0xB10C @stickies-v
The only problem I see with this approach is the code duplication. Which I think you solved right? Using:
```
const std::string details = std::format(
"transport={} version={} blocks={} peer={}{}{}",
TransportTypeAsString(pfrom.m_transport->GetInfo().transport_type),
pfrom.nVersion.load(),
int{peer->m_starting_height},
pfrom.GetId(),
(fLogIPs ? std::fo
...
(https://github.com/bitcoin/bitcoin/pull/34008#issuecomment-3621582615)
Personally I prefer this approach over https://github.com/bitcoin/bitcoin/pull/34018 @0xB10C @stickies-v
The only problem I see with this approach is the code duplication. Which I think you solved right? Using:
```
const std::string details = std::format(
"transport={} version={} blocks={} peer={}{}{}",
TransportTypeAsString(pfrom.m_transport->GetInfo().transport_type),
pfrom.nVersion.load(),
int{peer->m_starting_height},
pfrom.GetId(),
(fLogIPs ? std::fo
...
💬 Ataraxia009 commented on pull request "log: exempt all category-specific logs from ratelimiting when running with debug":
(https://github.com/bitcoin/bitcoin/pull/34018#issuecomment-3621587297)
Concept NAck
Users that are having issues that are not power users would also use the `-debug` option, to help submit logs.
if `-debug` is used with this implementation, it would stop rate-limiting of all category logs.
(https://github.com/bitcoin/bitcoin/pull/34018#issuecomment-3621587297)
Concept NAck
Users that are having issues that are not power users would also use the `-debug` option, to help submit logs.
if `-debug` is used with this implementation, it would stop rate-limiting of all category logs.
💬 Ataraxia009 commented on pull request "validation: Remove min_pow_checked arg in ProcessNewBlockHeaders":
(https://github.com/bitcoin/bitcoin/pull/34022#discussion_r2596019236)
If you are going to do this, then add a comment here saying `ProcessNewBlockHeaders` assumes that the caller has checked for min pow or something along those lines, for clarity
(https://github.com/bitcoin/bitcoin/pull/34022#discussion_r2596019236)
If you are going to do this, then add a comment here saying `ProcessNewBlockHeaders` assumes that the caller has checked for min pow or something along those lines, for clarity
💬 laanwj commented on pull request "guix: use GCC 14.3.0 over 13.3.0":
(https://github.com/bitcoin/bitcoin/pull/33775#issuecomment-3621690386)
FWIW: My bisect is not done yet (2 steps to go, it will likely finish today), but in the current commit range https://github.com/gcc-mirror/gcc/commit/1d82fc2e6824bf83159389729c31a942f7b91b04 `optimize std::vector::push_back` is the most suspicious as it affects vector appending. It's a change to `libstdc++-v3`, not the compiler, so it doesn't directly point at a root cause.
(https://github.com/bitcoin/bitcoin/pull/33775#issuecomment-3621690386)
FWIW: My bisect is not done yet (2 steps to go, it will likely finish today), but in the current commit range https://github.com/gcc-mirror/gcc/commit/1d82fc2e6824bf83159389729c31a942f7b91b04 `optimize std::vector::push_back` is the most suspicious as it affects vector appending. It's a change to `libstdc++-v3`, not the compiler, so it doesn't directly point at a root cause.
📝 Chand-ra opened a pull request: "fuzz: Add tests for `CCoinControl` methods"
(https://github.com/bitcoin/bitcoin/pull/34026)
The `ccoincontrol` fuzzer misses tests for `GetSequence()` and `GetScripts()`. Add them.
(https://github.com/bitcoin/bitcoin/pull/34026)
The `ccoincontrol` fuzzer misses tests for `GetSequence()` and `GetScripts()`. Add them.
📝 ryslan25500-cloud opened a pull request: "Create PROPOSAL-V34-Tetrad-Research.md"
(https://github.com/bitcoin/bitcoin/pull/34027)
Add PROPOSAL-V34-Tetrad-Research.md
Detailed technical proposal for a federated research mining pool with deterministic 27-symbol tetrad embedding in Bitcoin blocks.
This is the V34 version of the Tetrad Research Proposal — a fully open, scientific, and strictly non-esoteric project aimed at:
• Creating the first long-term deterministic pattern dataset embedded directly into Bitcoin blocks • Studying potential emergent structures in perfectly uniform pseudorandom sequences • Demonstrat
...
(https://github.com/bitcoin/bitcoin/pull/34027)
Add PROPOSAL-V34-Tetrad-Research.md
Detailed technical proposal for a federated research mining pool with deterministic 27-symbol tetrad embedding in Bitcoin blocks.
This is the V34 version of the Tetrad Research Proposal — a fully open, scientific, and strictly non-esoteric project aimed at:
• Creating the first long-term deterministic pattern dataset embedded directly into Bitcoin blocks • Studying potential emergent structures in perfectly uniform pseudorandom sequences • Demonstrat
...
💬 Sjors commented on issue "rpc: getrawtransaction lookup by witness txid":
(https://github.com/bitcoin/bitcoin/issues/34013#issuecomment-3621878889)
> for a while
I would not expect this to land before v31 in spring 2026, so by that time @Fi3 might have finished implementing IPC support and could just use the more performant #34020.
(https://github.com/bitcoin/bitcoin/issues/34013#issuecomment-3621878889)
> for a while
I would not expect this to land before v31 in spring 2026, so by that time @Fi3 might have finished implementing IPC support and could just use the more performant #34020.
💬 romanz commented on pull request "rest: allow reading partial block data from storage":
(https://github.com/bitcoin/bitcoin/pull/33657#discussion_r2596211587)
https://github.com/bitcoin/bitcoin/compare/bbea9636a3b11df1c79664e97ded7907bbacfcf3..9efccb0d3e84d83e0ec29a4b18b72019ffc3d5c3
(https://github.com/bitcoin/bitcoin/pull/33657#discussion_r2596211587)
https://github.com/bitcoin/bitcoin/compare/bbea9636a3b11df1c79664e97ded7907bbacfcf3..9efccb0d3e84d83e0ec29a4b18b72019ffc3d5c3
💬 romanz commented on pull request "rest: allow reading partial block data from storage":
(https://github.com/bitcoin/bitcoin/pull/33657#discussion_r2596211598)
https://github.com/bitcoin/bitcoin/compare/bbea9636a3b11df1c79664e97ded7907bbacfcf3..9efccb0d3e84d83e0ec29a4b18b72019ffc3d5c3
(https://github.com/bitcoin/bitcoin/pull/33657#discussion_r2596211598)
https://github.com/bitcoin/bitcoin/compare/bbea9636a3b11df1c79664e97ded7907bbacfcf3..9efccb0d3e84d83e0ec29a4b18b72019ffc3d5c3
💬 romanz commented on pull request "rest: allow reading partial block data from storage":
(https://github.com/bitcoin/bitcoin/pull/33657#discussion_r2596211612)
https://github.com/bitcoin/bitcoin/compare/bbea9636a3b11df1c79664e97ded7907bbacfcf3..9efccb0d3e84d83e0ec29a4b18b72019ffc3d5c3
(https://github.com/bitcoin/bitcoin/pull/33657#discussion_r2596211612)
https://github.com/bitcoin/bitcoin/compare/bbea9636a3b11df1c79664e97ded7907bbacfcf3..9efccb0d3e84d83e0ec29a4b18b72019ffc3d5c3
💬 romanz commented on pull request "rest: allow reading partial block data from storage":
(https://github.com/bitcoin/bitcoin/pull/33657#discussion_r2596211643)
https://github.com/bitcoin/bitcoin/compare/bbea9636a3b11df1c79664e97ded7907bbacfcf3..9efccb0d3e84d83e0ec29a4b18b72019ffc3d5c3
(https://github.com/bitcoin/bitcoin/pull/33657#discussion_r2596211643)
https://github.com/bitcoin/bitcoin/compare/bbea9636a3b11df1c79664e97ded7907bbacfcf3..9efccb0d3e84d83e0ec29a4b18b72019ffc3d5c3
💬 stickies-v commented on pull request "log: exempt all category-specific logs from ratelimiting when running with debug":
(https://github.com/bitcoin/bitcoin/pull/34018#issuecomment-3621890490)
> if `-debug` is used with this implementation, it would stop rate-limiting of all category logs.
Debug logs are (by design) much higher frequency than info/warning/error level logs. They have never been ratelimited, because for debugging purposes it's important to see what's happening without limitation. When running with `-debug`, the overwhelming majority of logs produced are debug logs. This PR doesn't change anything about any of that.
Info logs are typically much higher signal than d
...
(https://github.com/bitcoin/bitcoin/pull/34018#issuecomment-3621890490)
> if `-debug` is used with this implementation, it would stop rate-limiting of all category logs.
Debug logs are (by design) much higher frequency than info/warning/error level logs. They have never been ratelimited, because for debugging purposes it's important to see what's happening without limitation. When running with `-debug`, the overwhelming majority of logs produced are debug logs. This PR doesn't change anything about any of that.
Info logs are typically much higher signal than d
...
💬 stickies-v commented on pull request "log: exempt all category-specific logs from ratelimiting when running with debug":
(https://github.com/bitcoin/bitcoin/pull/34018#issuecomment-3621894437)
Force-pushed to address merge conflict from #29641, no other changes.
(https://github.com/bitcoin/bitcoin/pull/34018#issuecomment-3621894437)
Force-pushed to address merge conflict from #29641, no other changes.
💬 sedited commented on pull request "validation: Remove min_pow_checked arg in ProcessNewBlockHeaders":
(https://github.com/bitcoin/bitcoin/pull/34022#discussion_r2596217297)
Not sure if that is really useful. We have a lot of functions that assume some check has been done beforehand to protect against dos. I don't see this function being misused again in the future by mistake, headers-presync is just too well established in the code base at this point. Maybe a more complete description of what the function does would be useful for people reading this the first time, that could then in turn describe why a dos check should be done before calling. I you want, I could a
...
(https://github.com/bitcoin/bitcoin/pull/34022#discussion_r2596217297)
Not sure if that is really useful. We have a lot of functions that assume some check has been done beforehand to protect against dos. I don't see this function being misused again in the future by mistake, headers-presync is just too well established in the code base at this point. Maybe a more complete description of what the function does would be useful for people reading this the first time, that could then in turn describe why a dos check should be done before calling. I you want, I could a
...
💬 l0rinc commented on pull request "validation: Remove min_pow_checked arg in ProcessNewBlockHeaders":
(https://github.com/bitcoin/bitcoin/pull/34022#discussion_r2596215984)
I redid the change locally and I think this line is slightly different from the rest: could we do it in a separate commit before the refactor, so that after that the arg removal is a clean refactor?
We could also add `assert(min_pow_checked);` to the first commit, so that reviewers and CI can run all tests *with* the assert as an extra safety measure before the removal in the next one.
```C++
assert(min_pow_checked); // TODO removed in next commit
bool accepted{AcceptBlockHeader(header, state,
...
(https://github.com/bitcoin/bitcoin/pull/34022#discussion_r2596215984)
I redid the change locally and I think this line is slightly different from the rest: could we do it in a separate commit before the refactor, so that after that the arg removal is a clean refactor?
We could also add `assert(min_pow_checked);` to the first commit, so that reviewers and CI can run all tests *with* the assert as an extra safety measure before the removal in the next one.
```C++
assert(min_pow_checked); // TODO removed in next commit
bool accepted{AcceptBlockHeader(header, state,
...
👍 l0rinc approved a pull request: "validation: Remove min_pow_checked arg in ProcessNewBlockHeaders"
(https://github.com/bitcoin/bitcoin/pull/34022#pullrequestreview-3549208134)
Redid the change, ended up with basically the same state.
I suggest separating it into two commits to make make the change even safer to review, but I'm also fine with merging as-is (since I already did that locally).
Happy to re-ack if needed.
ACK beff6ff964c5c1b4a96af89daf6b2ddaede3464a
(https://github.com/bitcoin/bitcoin/pull/34022#pullrequestreview-3549208134)
Redid the change, ended up with basically the same state.
I suggest separating it into two commits to make make the change even safer to review, but I'm also fine with merging as-is (since I already did that locally).
Happy to re-ack if needed.
ACK beff6ff964c5c1b4a96af89daf6b2ddaede3464a
💬 romanz commented on pull request "rest: allow reading partial block data from storage":
(https://github.com/bitcoin/bitcoin/pull/33657#issuecomment-3621933771)
> > Could you provide a patch for me which would go through every single transaction in mainchain history and query it both ways (full block + new RPC) and assert that they're equal?
>
> Sounds good - will it be OK to write it as a Rust CLI tool to interact with bitcoind?
https://github.com/romanz/fetch-txs can be used for equivalence testing:
```
$ cargo run --release -- 900000 900010
Finished `release` profile [optimized] target(s) in 0.11s
Running `target/release/fetch-txs 9
...
(https://github.com/bitcoin/bitcoin/pull/33657#issuecomment-3621933771)
> > Could you provide a patch for me which would go through every single transaction in mainchain history and query it both ways (full block + new RPC) and assert that they're equal?
>
> Sounds good - will it be OK to write it as a Rust CLI tool to interact with bitcoind?
https://github.com/romanz/fetch-txs can be used for equivalence testing:
```
$ cargo run --release -- 900000 900010
Finished `release` profile [optimized] target(s) in 0.11s
Running `target/release/fetch-txs 9
...
💬 maflcko commented on pull request "test: p2p: check that peer's announced starting height is remembered":
(https://github.com/bitcoin/bitcoin/pull/33990#issuecomment-3621941650)
Yeah, I also wonder what the use-case here is or was.
If it is an estimate of how long the connection is established, there is the more accurate `conntime` field. If it exists to show which peer is ahead of whom, then the existing headers-sync and block-sync p2p flow seems a better way to figure it out, which is reported via the `synced_headers`, `presynced_headers`, or `synced_blocks` fields. If it exists just for debugging the sync logic, it seems best to just log it in the debug log.
...
(https://github.com/bitcoin/bitcoin/pull/33990#issuecomment-3621941650)
Yeah, I also wonder what the use-case here is or was.
If it is an estimate of how long the connection is established, there is the more accurate `conntime` field. If it exists to show which peer is ahead of whom, then the existing headers-sync and block-sync p2p flow seems a better way to figure it out, which is reported via the `synced_headers`, `presynced_headers`, or `synced_blocks` fields. If it exists just for debugging the sync logic, it seems best to just log it in the debug log.
...
💬 maflcko commented on pull request "rpc: combinerawtransaction now rejects unmergeable transactions":
(https://github.com/bitcoin/bitcoin/pull/31298#issuecomment-3621942550)
Are you still working on this?
(https://github.com/bitcoin/bitcoin/pull/31298#issuecomment-3621942550)
Are you still working on this?