💬 romanz commented on pull request "rest: allow reading partial block data from storage":
(https://github.com/bitcoin/bitcoin/pull/33657#issuecomment-3620482557)
> 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?

📝 BrandonOdiwuor converted_to_draft a pull request: "Feature: Use different datadirs for different signets"
(https://github.com/bitcoin/bitcoin/pull/29838)
Fixes https://github.com/bitcoin/bitcoin/issues/27494

When the `-signetchallenge` argument is provided, the hash-160 of the challenge is appended to the data directory name. This ensures that each signet has its own distinct data directory, following the naming convention `signet_XXXXXXX`.

Note:
The behaviour of the default signet remains unchanged

### Update:

When the `-signetchallenge` argument is provided, `the first 8 characters of the hash-160 of the challenge` are appended to
...

🤔 furszy reviewed a pull request: "p2p: avoid traversing blocks (twice) during IBD"
(https://github.com/bitcoin/bitcoin/pull/32730#pullrequestreview-3547824166)
> > That part was merged with #32827
>
> Ah right, forgot about that. Should have mentioned this directly, but the change I was looking for is in the txdownloadman. Was thinking that if this is rebased, we could add a patch for avoiding the additional iteration and filter calculation there in this pull request. But I guess I could also just open a separate PR for that?

Yeah. I'm not planning on working on it further. Have a few things on my plate already. Feel free to tag me as a reviewer.
...

💬 sedited commented on pull request "Add util::Expected (std::expected)":
(https://github.com/bitcoin/bitcoin/pull/34006#discussion_r2595077727)
Can't this be released when creating the `ProxyClient`? Looks like `destroy_connection` being true means the client is taking ownership of the connection.

💬 arejula27 commented on pull request "refactor: Add util::Result failure types and ability to merge result values":
(https://github.com/bitcoin/bitcoin/pull/25665#discussion_r2595092169)
I removed this include and it continue compiling, are we sure this is required? I executed the following commands:
```bash
➜ bitcoin (25665) % git log -1 --format=%H 17:21
f0aff63b5ad51566e626d5b24eee08eb81df54a1
➜ bitcoin (25665) % git diff src/util/result.cpp 17:21
diff --git a/src/ut
...

💬 plebhash commented on issue "rpc: getrawtransaction lookup by witness txid":
(https://github.com/bitcoin/bitcoin/issues/34013#issuecomment-3620660349)
I see @Fi3 manifested interest for this, so I'd assume he still wants his JDS implementation to rely on a poll-style RPC-based design for a while

on SRI we're aiming for a stream-based design under the efforts for https://github.com/stratum-mining/sv2-apps/issues/26

so IPC would be desirable, but as a potential replacement for ZMQ `rawtx` subscription, and not as a in-place replacement for `getrawtransaction`

💬 sedited commented on pull request "refactor: Add util::Result failure types and ability to merge result values":
(https://github.com/bitcoin/bitcoin/pull/25665#discussion_r2595120111)
We usually treat missing or extra includes as nits. There is an iwyu run as part of the tidy CI job. See the logs here: https://github.com/bitcoin/bitcoin/actions/runs/19279986116/job/55128814542?pr=25665#step:9:13821 .

💬 arejula27 commented on pull request "refactor: Add util::Result failure types and ability to merge result values":
(https://github.com/bitcoin/bitcoin/pull/25665#discussion_r2595126060)
I reviewed the [algoirhm lib](https://cplusplus.com/reference/algorithm/) and looks like none of its functions is used on this file. Maybe was not correctly cleanup

💬 arejula27 commented on pull request "refactor: Add util::Result failure types and ability to merge result values":
(https://github.com/bitcoin/bitcoin/pull/25665#discussion_r2595139565)
Does this mean that I should not review includes? I am starting to contribute, sorry if this is not relevant

💬 arejula27 commented on pull request "refactor: Add util::Result failure types and ability to merge result values":
(https://github.com/bitcoin/bitcoin/pull/25665#discussion_r2595139603)
Does this mean that I should not review includes? I am starting to contribute, sorry if this is not relevant

💬 sedited commented on pull request "refactor: Add util::Result failure types and ability to merge result values":
(https://github.com/bitcoin/bitcoin/pull/25665#discussion_r2595150404)
If it is important to you, it is enough to link the respective lines in the iwyu report.

💬 sedited commented on pull request "kernel: Add block header support and validation":
(https://github.com/bitcoin/bitcoin/pull/33822#discussion_r2595152765)
Removed the arg in https://github.com/bitcoin/bitcoin/pull/34022.

💬 arejula27 commented on pull request "refactor: Add util::Result failure types and ability to merge result values":
(https://github.com/bitcoin/bitcoin/pull/25665#discussion_r2595158431)
Thx🫡, I guess this conversation can be marked as resolved then

💬 arejula27 commented on pull request "refactor: Add util::Result failure types and ability to merge result values":
(https://github.com/bitcoin/bitcoin/pull/25665#discussion_r2595158488)
Thx🫡, I guess this conversation can be marked as resolved then

💬 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-3620731752)
Wouldn't this cause all logs for all categories to be not rate limited mid i do put just -debug with no specific category?

💬 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-3620735787)
> Wouldn't this cause all logs for all categories to be not rate limited if i do put just -debug with no specific category?

If this is true, it seems like we wouldn't cause more friction for users to submit in depth logs

💬 andrewtoth commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#issuecomment-3620885363)
re-ACK f391296b17a755153960e0afc736df37d1d5fb1e

Minor changes since last review.

💬 andrewtoth commented on pull request "rest: allow reading partial block data from storage":
(https://github.com/bitcoin/bitcoin/pull/33657#discussion_r2595369704)
We can make this a little easier to parse with less nesting if we did:
```C++
std::pair<size_t, size_t> block_part;
if (const auto opt_offset{ToIntegral<size_t>(req->GetQueryParameter("offset").value_or(""))}) {
block_part.first = *opt_offset;
} else {
return RESTERR(req, HTTP_BAD_REQUEST, "Block part offset missing or invalid");
}
if (const auto opt_size{ToIntegral<size_t>(req->GetQueryParameter("size").value_or(""))}) {

...

💬 andrewtoth commented on pull request "rest: allow reading partial block data from storage":
(https://github.com/bitcoin/bitcoin/pull/33657#discussion_r2595371448)
nit
```suggestion
const auto block_data{ReadRawBlock(pos)};
```

💬 andrewtoth commented on pull request "rest: allow reading partial block data from storage":
(https://github.com/bitcoin/bitcoin/pull/33657#discussion_r2595371025)
nit
```suggestion
if (const auto block_data{m_chainman.m_blockman.ReadRawBlock(block_pos)}) {
```