💬 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

🚀 fanquake merged a pull request: "test: Avoid duplicate curl call in get_previous_releases.py"
(https://github.com/bitcoin/bitcoin/pull/30703)

🤔 furszy reviewed a pull request: "Wallet, Bugfix: Lock Wallet Context mutex Before Adding/Removing Settings"
(https://github.com/bitcoin/bitcoin/pull/30697#pullrequestreview-2258765600)
I don't think using the context wallet's mutex is the best approach for this issue. The problem lies in how we update settings, which is not atomic. Today, the race is in the wallets flow, but tomorrow it could easily affect another settings option.

Updates are executed in three separate steps:
1) Obtain settings value while acquiring the settings lock.
2) Modify settings value.
3) Overwrite settings value while acquiring the settings lock.

This process is not thread-safe. Different thr
...

💬 furszy commented on pull request "Wallet, Bugfix: Lock Wallet Context mutex Before Adding/Removing Settings":
(https://github.com/bitcoin/bitcoin/pull/30697#discussion_r1730021378)
This could be shorter:
```
// This test verifies that wallet settings can be added and removed
// concurrently, ensuring no race conditions occur during either process.
BOOST_FIXTURE_TEST_CASE(write_wallet_settings_concurrently, TestingSetup)
{
WalletContext context;
context.chain = m_node.chain.get();
const auto NUM_WALLETS{5};
// Since we're counting the number of wallets, ensure we start without any.
BOOST_REQUIRE(context.chain->getRwSetting("wallet").isNull());
...

📝 jamesob opened a pull request: "rpc: add getdescriptoractivity"
(https://github.com/bitcoin/bitcoin/pull/30708)
The RPC command `scanblocks` provides a useful way to get a set of blockhashes that have activity relevant to a set of descriptors (`relevant_blocks`). However actually extracting the activity from those blocks is left as an exercise to the end user.

This process involves not only generating the (potentially ranged) set of scripts for the descriptor set on the client side (maybe via `deriveaddresses`), but then the user must retrieve each block's contents one-by-one using `getblock <hash>`,
...

👍 theStack approved a pull request: "Use MiniWallet in functional test rpc_signrawtransactionwithkey."
(https://github.com/bitcoin/bitcoin/pull/30701#pullrequestreview-2258787624)
lgtm ACK 96d34fb2e493a2c37907e76808215c5ab0c3cf70

🚀 fanquake merged a pull request: "test: replace deprecated secp256k1 context flags usage"
(https://github.com/bitcoin/bitcoin/pull/30687)

🚀 fanquake merged a pull request: "fuzz: remove repeated word in note"
(https://github.com/bitcoin/bitcoin/pull/30651)

🤔 tdb3 reviewed a pull request: "rpc: add getdescriptoractivity"
(https://github.com/bitcoin/bitcoin/pull/30708#pullrequestreview-2258846712)
Concept ACK

This seems to be a great way to simplify the chain of RPC commands needed to obtain targeted transaction history.

Thought about a potential alternative to update/enhance `scanblocks` instead of adding a new RPC (with the advantage of not needing to provide block hashes). Adding `getdescriptoractivity` seems like a better solution, since it avoids breaking RPC compatibility. Carrying blockhashes from `scanblocks` to `getdescriptoractivity` seems like a reasonably price to pay fo
...

💬 l0rinc commented on pull request "refactor: Replace ParseHex with consteval ""_hex literals":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1730107698)
I also like the suggestion, but there are other hex conversion methods in uint256 which could use some compile-time love - I'm fine with doing it in a separate PR.