💬 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?
💬 maflcko commented on pull request "Add util::Expected (std::expected)":
(https://github.com/bitcoin/bitcoin/pull/34006#discussion_r2596258697)
In modern languages like Rust, yes. But not in C++, because there is no order of evaluation of the function arguments.
So `make_unique(client->m_rpc, client.release())` will lead to a nullptr deref (segfault).
I could rewrite this to use `new ProxyClient{client->m_rpc, client.release()}`, because for braced-init, the order is defined, but I am not sure if this is better, because it violates https://clang.llvm.org/extra/clang-tidy/checks/modernize/make-unique.html
(https://github.com/bitcoin/bitcoin/pull/34006#discussion_r2596258697)
In modern languages like Rust, yes. But not in C++, because there is no order of evaluation of the function arguments.
So `make_unique(client->m_rpc, client.release())` will lead to a nullptr deref (segfault).
I could rewrite this to use `new ProxyClient{client->m_rpc, client.release()}`, because for braced-init, the order is defined, but I am not sure if this is better, because it violates https://clang.llvm.org/extra/clang-tidy/checks/modernize/make-unique.html
💬 maflcko commented on pull request "Add util::Expected (std::expected)":
(https://github.com/bitcoin/bitcoin/pull/34006#discussion_r2596259276)
> I would like you to consider adding **directly** as I mentioned, as you already did some small text corrections on `results.h`
Not sure. I am not touching this line, so I'll leave as-is for now.
(https://github.com/bitcoin/bitcoin/pull/34006#discussion_r2596259276)
> I would like you to consider adding **directly** as I mentioned, as you already did some small text corrections on `results.h`
Not sure. I am not touching this line, so I'll leave as-is for now.
💬 maflcko commented on pull request "log: exempt all category-specific logs from ratelimiting when running with debug":
(https://github.com/bitcoin/bitcoin/pull/34018#issuecomment-3621967700)
`LogPrintLevel` isn't really documented in the dev notes and the unit test name `logging_LogPrintMacrosDeprecated` seems to indicate it is deprecated. This change here seems to put a use-case to it, so it could make sense to document and test it? Though, I wonder instead of using the `LogPrintLevel` macro, it could make more sense to just add a new `LogInfo` macro that mirrors the `LogDebug` signature? I can't really see a use-case to have high-volume warnings or errors, so that they run into ra
...
(https://github.com/bitcoin/bitcoin/pull/34018#issuecomment-3621967700)
`LogPrintLevel` isn't really documented in the dev notes and the unit test name `logging_LogPrintMacrosDeprecated` seems to indicate it is deprecated. This change here seems to put a use-case to it, so it could make sense to document and test it? Though, I wonder instead of using the `LogPrintLevel` macro, it could make more sense to just add a new `LogInfo` macro that mirrors the `LogDebug` signature? I can't really see a use-case to have high-volume warnings or errors, so that they run into ra
...
💬 ajtowns commented on pull request "log: don't rate-limit "new peer" with -debug=net":
(https://github.com/bitcoin/bitcoin/pull/34008#discussion_r2596296831)
Yeah. If it's not obvious, the whole thing would look something like:
```c++
auto new_peer_info = [&]() {
// lambda so that this info is only calculated if needed
const auto mapped_as{m_connman.GetMappedAS(pfrom.addr)};
return strprintf("transport: %s version: %d, blocks=%d, peer=%d%s%s\n",
TransportTypeAsString(pfrom.m_transport->GetInfo().transport_type),
pfrom.nVersion.load(), peer->m_starting
...
(https://github.com/bitcoin/bitcoin/pull/34008#discussion_r2596296831)
Yeah. If it's not obvious, the whole thing would look something like:
```c++
auto new_peer_info = [&]() {
// lambda so that this info is only calculated if needed
const auto mapped_as{m_connman.GetMappedAS(pfrom.addr)};
return strprintf("transport: %s version: %d, blocks=%d, peer=%d%s%s\n",
TransportTypeAsString(pfrom.m_transport->GetInfo().transport_type),
pfrom.nVersion.load(), peer->m_starting
...
💬 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-3622056086)
> > 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 hi
...
(https://github.com/bitcoin/bitcoin/pull/34018#issuecomment-3622056086)
> > 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 hi
...
💬 romanz commented on pull request "rest: allow reading partial block data from storage":
(https://github.com/bitcoin/bitcoin/pull/33657#discussion_r2596332437)
Fixed in 7802c1f9ca09b5669b8400e96baeadc238fa9df2
(https://github.com/bitcoin/bitcoin/pull/33657#discussion_r2596332437)
Fixed in 7802c1f9ca09b5669b8400e96baeadc238fa9df2
💬 romanz commented on pull request "rest: allow reading partial block data from storage":
(https://github.com/bitcoin/bitcoin/pull/33657#discussion_r2596332557)
Fixed in 7802c1f9ca09b5669b8400e96baeadc238fa9df2
(https://github.com/bitcoin/bitcoin/pull/33657#discussion_r2596332557)
Fixed in 7802c1f9ca09b5669b8400e96baeadc238fa9df2
💬 romanz commented on pull request "rest: allow reading partial block data from storage":
(https://github.com/bitcoin/bitcoin/pull/33657#discussion_r2596332829)
Used `util::Expected` from fa114be27b17ed32c1d9a7106f313a0df8755fa2
(https://github.com/bitcoin/bitcoin/pull/33657#discussion_r2596332829)
Used `util::Expected` from fa114be27b17ed32c1d9a7106f313a0df8755fa2
💬 romanz commented on pull request "rest: allow reading partial block data from storage":
(https://github.com/bitcoin/bitcoin/pull/33657#discussion_r2596333260)
Fixed in 71c7b747091fce045dbf25e4e0cfc0e6a4ad6435
(https://github.com/bitcoin/bitcoin/pull/33657#discussion_r2596333260)
Fixed in 71c7b747091fce045dbf25e4e0cfc0e6a4ad6435
💬 romanz commented on pull request "rest: allow reading partial block data from storage":
(https://github.com/bitcoin/bitcoin/pull/33657#discussion_r2596333791)
Fixed in 71c7b747091fce045dbf25e4e0cfc0e6a4ad6435
(https://github.com/bitcoin/bitcoin/pull/33657#discussion_r2596333791)
Fixed in 71c7b747091fce045dbf25e4e0cfc0e6a4ad6435
💬 romanz commented on pull request "rest: allow reading partial block data from storage":
(https://github.com/bitcoin/bitcoin/pull/33657#discussion_r2596334064)
Fixed in 71c7b747091fce045dbf25e4e0cfc0e6a4ad6435
(https://github.com/bitcoin/bitcoin/pull/33657#discussion_r2596334064)
Fixed in 71c7b747091fce045dbf25e4e0cfc0e6a4ad6435