💬 fjahr commented on pull request "assumeutxo: Add dumptxoutset height param, remove shell scripts":
(https://github.com/bitcoin/bitcoin/pull/29553#issuecomment-2307876124)
I addressed @Sjors 's feedback and rebased so the behavior can be tested with the mainnet params.

> This can wait for a followup, but can you add -rpcclienttimeout=0 to all the examples?

Done

> But that's because this PR wasn't rebased after https://github.com/bitcoin/bitcoin/pull/28553 got merged.

Rebased for easier testing.

👍 pablomartin4btc approved a pull request: "build: Introduce CMake-based build system"
(https://github.com/bitcoin/bitcoin/pull/30454#pullrequestreview-2258177952)
tACK 41051290ab3b6c36312cec26a27f787cea9961b4

Cross built for Win11 on WSL 24.04, performed `install`, `deploy`, run `ctest` and `test_bitcoin` on both WSL and WIndows.

Small details regarding the docs:
- In "Building for 64-bit Windows" doc, building depends got changed to `gmake`, but not in depends doc.
- Also there's no reference to `ctest` in `build-unix.md` except at the end as part of the "Setup and Build Example: Arch Linux" , should not be at the beginning in the "To Build" sect
...

✅ achow101 closed a pull request: "contrib: add dockerfile for building image fron source code"
(https://github.com/bitcoin/bitcoin/pull/30702)

💬 ryanofsky commented on pull request "refactor: Replace ParseHex with consteval ""_hex literals":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1729584258)
re: https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1728787408

This is an interesting idea. I kind of think now that we have support for these suffixes, we should just add an `_rhex` reverse hex suffix and drop this constructor entirely, avoid the footgun of having two constructors where one reverses its input and the other does not.

Also, IMO if adopting this suggestion it would be a little better to keep detail namespace than to expose Hex class as something that would be reaso
...

💬 ryanofsky commented on pull request "refactor: Replace ParseHex with consteval ""_hex literals":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1729536195)
re: https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1729064946

> I want to explicitly enforce compile time where possible, hence the consteval.

I don't think this is a good idea. IMO, the interface should simple and consistent, not noisy with unnecessary / unexplained differences that obscure more real and relevant differences between the functions. It's not good to expose internal implementation details unnecessarily or to tell compilers how to do their jobs unnecessarily. The c
...

👍 tdb3 approved a pull request: "test: Add time-timewarp-attack boundary cases"
(https://github.com/bitcoin/bitcoin/pull/30698#pullrequestreview-2258270996)
ACK 31378d44f44cabc576bf92ddd61afde4b528de77

Great addition (testing around `MAX_FUTURE_BLOCK_TIME - MAX_TIMEWARP`).
Tested locally (and sanity checked that the assert would catch removing the `-1` from `bad_block.nTime = t + MAX_FUTURE_BLOCK_TIME - MAX_TIMEWARP - 1`).

Left one comment, but it doesn't block the ACK.

💬 tdb3 commented on pull request "test: Add time-timewarp-attack boundary cases":
(https://github.com/bitcoin/bitcoin/pull/30698#discussion_r1729632270)
I'm not immediately seeing why the previous check (for the `bad_block.nTime = t` case) is needed anymore now that the check below is being added (`bad_block.nTime = t + MAX_FUTURE_BLOCK_TIME - MAX_TIMEWARP - 1` case). Maybe I'm overlooking something?

If not, then could add a commit that removes the check for the `bad_block.nTime = t` case.

💬 hodlinator commented on pull request "refactor: Replace ParseHex with consteval ""_hex literals":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1729787316)
Not sure about how `_rhex` would work, if it returned `std::array<std::byte, N>` one could accidentally substitute for `_hex`. Keeping `Hex` in `detail` for now though.

💬 hodlinator commented on pull request "refactor: Replace ParseHex with consteval ""_hex literals":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1729787403)
Done!

💬 hodlinator commented on pull request "refactor: Replace ParseHex with consteval ""_hex literals":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1729787874)
As long as `Hex` is `consteval` I'm happy. Changed the literals that could be into `constexpr` and added comment about the `inline` outlier.

💬 hodlinator commented on pull request "refactor: Replace ParseHex with consteval ""_hex literals":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1729902294)
Bummer, broke Win64.

Aiming to back out that change later today.

👍 TheCharlatan approved a pull request: "Have createNewBlock() return a BlockTemplate interface"
(https://github.com/bitcoin/bitcoin/pull/30440#pullrequestreview-2258594732)
ACK f3b6e728b9e9b16c71849ad044d3ef3e8240be79

💬 TheCharlatan commented on pull request "Have createNewBlock() return a BlockTemplate interface":
(https://github.com/bitcoin/bitcoin/pull/30440#discussion_r1729875816)
Nit: Alphabetical order for the includes. There are some other minor clang-format issues, maybe run the commit through `clang-format-diff`? As far as I know we also don't have a tool to check the includes of these interface headers, but it is possible to check them manually:
```
interfaces/mining.h should add these lines:
#include <stdint.h> // for int64_t
#include <vector> // for vector
namespace node { struct BlockCreateOptions; }

interfaces/mining.h
...

💬 mzumsande commented on pull request "validation: improve m_best_header tracking":
(https://github.com/bitcoin/bitcoin/pull/30666#discussion_r1729934404)
I think that this is not a complete fix. This would only mark indexes between `m_best_header` and the failed block as `BLOCK_FAILED_CHILD`, but if this was not a linear chain and there were forked blocks, those wouldn't be marked as `BLOCK_FAILED_CHILD`. I think I'll change the logic to also set `BLOCK_FAILED_CHILD` while iterating over the entire block index.

💬 mzumsande commented on pull request "validation: improve m_best_header tracking":
(https://github.com/bitcoin/bitcoin/pull/30666#issuecomment-2308355302)
Thanks for the reviews so far! Ill address feedback soon, plus I want to rework 7d42b0e4c2722b398c73695d5547fbf8bd2ae01c, see comment above. I'm currently on holidays, so it might take a few days until I make the changes.

One more issue I've noticed while writing the tests this PR is that if we connect an invalid block received via p2p, `InvalidChainFound()` is called twice:
The first time in `ConnectTip()`, via `InvalidBlockFound()` [here](https://github.com/bitcoin/bitcoin/blob/c81c6bf65b3
...

⚠️ github12101 opened an issue: "b-msghand[4988] general protection fault"
(https://github.com/bitcoin/bitcoin/issues/30706)
### Is there an existing issue for this?

- [X] I have searched the existing issues

### Current behaviour

Hello dear devs, I caught segfault today, running latest Bitcoin Code in full node mode on AMD64 Linux server.

### Expected behaviour

no segfault

### Steps to reproduce

I had not seen bitcoind crash for a long time. It's a rare one I guess.

### Relevant log output

dmesg:
`traps: b-msghand[4988] general protection fault ip:55783548b790 sp:7f57627f3978 error:0 in bitcoind[557834c96000
...

💬 github12101 commented on issue "Fatal LevelDB error: Corruption: block checksum mismatch on Linux ext4 SATA SSDs":
(https://github.com/bitcoin/bitcoin/issues/30692#issuecomment-2308365675)
I am also inclined to say this is hardware error. I had these and it turned out to be unreliable RAM.
Put that database on Btrfs, it should never have checksum corruption error there. If it does, on Btrfs, and you have kernel errors about checksum, then this is 100% hardware problem.
I run full node on Btrfs and I don't have any problems.

💬 hodlinator commented on pull request "refactor: Replace ParseHex with consteval ""_hex literals":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1729995642)
Think there may still be something to @stickies-v intuition here. Maybe the `consteval` `base_blob`-ctor could somehow be implemented in terms of `""_hex_u8` reverse-copied into `base_blob::m_data` instead of only borrowing `ConstevalHexDigit`. Don't have the stamina to reconcile `Hex<N>` with `std::string_view` right now though.

💬 l0rinc commented on pull request "refactor: Replace ParseHex with consteval ""_hex literals":
(https://github.com/bitcoin/bitcoin/pull/30377#issuecomment-2308415965)
[docs + const mostly](https://github.com/bitcoin/bitcoin/compare/df92661..e63d7e54d26ddabedc5cd2cb8e1180da520fd063)

ACK e63d7e54d26ddabedc5cd2cb8e1180da520fd063

💬 l0rinc commented on pull request "refactor: Replace ParseHex with consteval ""_hex literals":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1730008317)
nit: to be fair, this comment confuses me even more