💬 stringintech commented on pull request "kernel: Add block header support and validation":
(https://github.com/bitcoin/bitcoin/pull/33822#discussion_r2532051597)
Indentation can be fixed here.

💬 stringintech commented on pull request "kernel: Add block header support and validation":
(https://github.com/bitcoin/bitcoin/pull/33822#discussion_r2532058549)
This can be simplified:
```suggestion
return btck_BlockValidationState::create();
```

💬 stringintech commented on pull request "kernel: Add block header support and validation":
(https://github.com/bitcoin/bitcoin/pull/33822#discussion_r2532068254)
Not sure but "Must be destroyed ..." docs may not be necessary as we have not documented this for the rest of the owned returned pointers (e.g. `btck_Block*`).

<details>
<summary>diff</summary>

```diff
diff --git a/src/kernel/bitcoinkernel.h b/src/kernel/bitcoinkernel.h
index 89b69b849d..a980000968 100644
--- a/src/kernel/bitcoinkernel.h
+++ b/src/kernel/bitcoinkernel.h
@@ -918,7 +918,7 @@ BITCOINKERNEL_API const btck_BlockTreeEntry* BITCOINKERNEL_WARN_UNUSED_RESULT bt
* @brief Re
...

💬 stringintech commented on pull request "kernel: Add block header support and validation":
(https://github.com/bitcoin/bitcoin/pull/33822#discussion_r2532063484)
We have usually documented the non-owned pointers this way: `The returned X is unowned and only valid for the lifetime of Y.`
```suggestion
/**
* @brief Get the previous block hash from block header. The returned hash is unowned and only valid for the lifetime of the block header.
*
* @param[in] header Non-null header
* @return Previous block hash
*/
BITCOINKERNEL_API const btck_BlockHash* BITCOINKERNEL_WARN_UNUSED_RESULT btck_block_header_get_prev_hash(
const b
...

💬 romanz commented on pull request "rest: allow reading partial block data from storage":
(https://github.com/bitcoin/bitcoin/pull/33657#discussion_r2532087454)
The above suggestion fails to compile on my machine:
```
src/rest.cpp: In function ‘bool rest_block_part(const std::any&, HTTPRequest*, const std::string&)’:
src/rest.cpp:505:43: error: expected ‘{’ before ‘[’ token
505 | } catch (const std::runtime_error& e) [[unlikely]] {
| ^

$ gcc --version
gcc (Ubuntu 13.3.0-6ubuntu2~24.04) 13.3.0
```

💬 andrewtoth commented on pull request "rest: allow reading partial block data from storage":
(https://github.com/bitcoin/bitcoin/pull/33657#discussion_r2532090415)
Ahh I didn't test these suggestions before submitting. I wasn't sure if we could have that annotation after a catch block. Please ignore.

💬 romanz commented on pull request "rest: allow reading partial block data from storage":
(https://github.com/bitcoin/bitcoin/pull/33657#issuecomment-3538991279)
Thanks @andrewtoth!
I have applied the patch below on e14650967d, but it seems that it didn't improve the following benchmark performance (reading the smallest possible amount of data from a block):
```
$ BLOCKHASH=00000000000000000001878540abd91b4dd591287ee58de73a163b698748f23b
$ ab -q -k -c 1 -n 100000 "http://localhost:8332/rest/blockpart/$BLOCK.bin?offset=0&size=1"
```
<details>

<summary> Patch </summary>

```diff
diff --git a/src/node/blockstorage.cpp b/src/node/blockstorage.cp
...

⚠️ alexandre-leng opened an issue: "Improve Bitcoin’s resilience to large-scale power grid failures and Carrington-type solar storms"
(https://github.com/bitcoin/bitcoin/issues/33885)
### Please describe the feature you'd like to see added.

Hi,
This is a feature request to explore options for improving Bitcoin’s resilience during large-scale, long-duration power or network outages (e.g., Carrington-type solar storms).
Such events could isolate regions for days or weeks, causing separate chain histories that later need reconciliation.

The request is to consider small, consensus-compatible improvements in documentation, defaults, or optional tools to help nodes operate on deg
...

💬 alexandre-leng commented on issue "Improve Bitcoin’s resilience to large-scale power grid failures and Carrington-type solar storms":
(https://github.com/bitcoin/bitcoin/issues/33885#issuecomment-3539127615)
This is just a starting point to spark some thought.

💬 fjahr commented on pull request "Embedded ASMap [3/3]: Build binary dump header file":
(https://github.com/bitcoin/bitcoin/pull/28792#discussion_r2531999067)
Addressed in #33878

💬 fjahr commented on pull request "Embedded ASMap [3/3]: Build binary dump header file":
(https://github.com/bitcoin/bitcoin/pull/28792#discussion_r2532002918)
Addressed in https://github.com/bitcoin/bitcoin/pull/33878

💬 fjahr commented on pull request "Embedded ASMap [3/3]: Build binary dump header file":
(https://github.com/bitcoin/bitcoin/pull/28792#discussion_r2532004290)
Renamed parameter in https://github.com/bitcoin/bitcoin/pull/33878

💬 fjahr commented on pull request "Embedded ASMap [3/3]: Build binary dump header file":
(https://github.com/bitcoin/bitcoin/pull/28792#discussion_r2532010522)
Addressed in https://github.com/bitcoin/bitcoin/pull/33878

💬 fjahr commented on pull request "Embedded ASMap [3/3]: Build binary dump header file":
(https://github.com/bitcoin/bitcoin/pull/28792#discussion_r2532010167)
Probably not a bad idea in principle but the nature of the test changes with your suggested code, so I would like to manage the PR scope a bit and keep this for a follow-up.

💬 fjahr commented on pull request "Embedded ASMap [3/3]: Build binary dump header file":
(https://github.com/bitcoin/bitcoin/pull/28792#discussion_r2532054758)
Addressed in #33878

> (Also removes data from file-global scope).

Not a problem because of the anonymous namespace I think.

💬 fjahr commented on pull request "Embedded ASMap [3/3]: Build binary dump header file":
(https://github.com/bitcoin/bitcoin/pull/28792#discussion_r2532014755)
Addressed in https://github.com/bitcoin/bitcoin/pull/33878

💬 fjahr commented on pull request "Embedded ASMap [3/3]: Build binary dump header file":
(https://github.com/bitcoin/bitcoin/pull/28792#discussion_r2532014382)
I have been working on improved documentation. Please check the new commit in https://github.com/bitcoin/bitcoin/pull/33878 to see if that is fixing this completely.

💬 fjahr commented on pull request "Embedded ASMap [3/3]: Build binary dump header file":
(https://github.com/bitcoin/bitcoin/pull/28792#discussion_r2532224663)
Addressed in https://github.com/bitcoin/bitcoin/pull/33878

💬 fjahr commented on pull request "Embedded ASMap [3/3]: Build binary dump header file":
(https://github.com/bitcoin/bitcoin/pull/28792#discussion_r2532022415)
Done in #33878

💬 fjahr commented on pull request "Embedded ASMap [3/3]: Build binary dump header file":
(https://github.com/bitcoin/bitcoin/pull/28792#issuecomment-3539393301)
All the feedback that applied to https://github.com/bitcoin/bitcoin/pull/33878 has now been addressed there. I am marking these comments as resolved and if there are further notes on these please move the conversation over there. Thanks!