💬 adamandrews1 commented on issue "Feature request: Backup of datadir state":
(https://github.com/bitcoin/bitcoin/issues/31324#issuecomment-2486838594)
If this goes forward, I think the backup folder path should be configurable and should have a default path outside the datadir.
Default would be disabled.
I don't see the need to keep two backup files. The backup operation should be atomic and replace the old backup file. If it fails, the old backup file should remain undisturbed.
I think API should be in terms of block height.
`-autobackup=100` would attempt to re-write the backup on new block discovery if the new block has height > 100 fro
...

👍 theStack approved a pull request: "descriptor: Move filling of keys from `DescriptorImpl::MakeScripts` to `PubkeyProvider::GetPubKey`"
(https://github.com/bitcoin/bitcoin/pull/31243#pullrequestreview-2446762914)
re-ACK 186bc75985a0afa96eba3c5bfea09e22fe3ec2c8 🗒️

🤔 jonatack reviewed a pull request: "Fuzz: extend CConnman tests"
(https://github.com/bitcoin/bitcoin/pull/28584#pullrequestreview-2446770519)
utACK c97d49628a78aac9a65f2bd1ddc733b0b425090b

💬 stickies-v commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r1849132153)
Sorry, I forgot to include the diff which updates the macro (and 1 instance to show it compiles):

<details>
<summary>git diff on 6c9121f790</summary>

```diff
diff --git a/src/kernel/bitcoinkernel.h b/src/kernel/bitcoinkernel.h
index 9e6bf127db..67248349e2 100644
--- a/src/kernel/bitcoinkernel.h
+++ b/src/kernel/bitcoinkernel.h
@@ -31,9 +31,9 @@
#define BITCOINKERNEL_WARN_UNUSED_RESULT
#endif
#if !defined(BITCOINKERNEL_BUILD) && defined(__GNUC__) && BITCOINKERNEL_GNUC_PREREQ(3,
...

⚠️ pbies opened an issue: "Maximum dbcache too small"
(https://github.com/bitcoin/bitcoin/issues/31326)
### Is there an existing issue for this?

- [X] I have searched the existing issues

### Current behaviour

28.0 gives 'out of space' and hangs when dbcache is set to 16384 and full chainstate -reindex is run.

### Expected behaviour

Should -reindex complete the reindexing.

### Steps to reproduce

Have whole blockchain on disk, txindex=1, dbcache=16384 and reindex whole chainstate with command line -reindex option.

### Relevant log output

2024-11-18T17:15:31Z Cache size (16396074160) exceeds
...

💬 pbies commented on issue "Maximum dbcache too small":
(https://github.com/bitcoin/bitcoin/issues/31326#issuecomment-2486887198)
Seems like the issue is also here: https://github.com/bitcoin/bitcoin/issues/27599

📝 GabrieleBocchi opened a pull request: "doc: Correct PR Review Club frequency from weekly to monthly"
(https://github.com/bitcoin/bitcoin/pull/31327)
The PR Review Club is mentioned as weekly in the CONTRIBUTING.md file, but it is held monthly as per the official [Bitcoin Core PR Review Club website](https://bitcoincore.reviews/). This PR updates the documentation to reflect the correct frequency.

💬 hodlinator commented on pull request "test: Revert to random path element":
(https://github.com/bitcoin/bitcoin/pull/31317#discussion_r1849149324)
Having `g_rng_temp_path_init` be called as soon as this compilation unit is included in a binary isn't ideal but still acceptable (`BasicTestingSetup` may not be used in a given run).

Good call on removing the defunct comment before `SeedRandomForTest()` as well.

👍 hodlinator approved a pull request: "test: Revert to random path element"
(https://github.com/bitcoin/bitcoin/pull/31317#pullrequestreview-2446803872)
ACK faaaf59f71ede057b2c1d369ef8db973c2f2dbc2

Tested cherrypicking 8e5f8a4f775b93d30b86d28405886eea232cf875 on top and re-running test from https://github.com/bitcoin/bitcoin/pull/31317/files#r1848404959, confirming that the second commit does in fact make paths random.

💬 achow101 commented on issue "Maximum dbcache too small":
(https://github.com/bitcoin/bitcoin/issues/31326#issuecomment-2486913781)
The "hanging" is caused by the cache flushing. For a 16GB cache, that can take quite a while, depending on your hardware. The only solution is to be patient and wait for it to finish flushing. If you turn on additional debug logging, you should see consistent log lines about it flushing entries to disk.

Increasing the cache will only make that "hanging" worse as the cache will be even bigger and so even more data to be flushed at once.

#28358 drops the dbcache limit, but that won't make th
...

🤔 hodlinator requested changes to a pull request: "policy: ephemeral dust followups"
(https://github.com/bitcoin/bitcoin/pull/31279#pullrequestreview-2445579323)
Reviewed 5679f398e2a3ab8315d39ee674ccb8ad6a8f73c7

Especially like the refactor of `CheckEphemeralSpends()` with less surprising return/out-values.

Good to see `LIMITED_WHILE(fuzzed_data_provider.remaining_bytes() > 0` in more places than I suggested.

Suggested changes aside from inline comments:

### 03ec3f6230f440b264abe3aa42a57fdd29bd3ba3 commit message

```diff
- Implement GetDust to replace HasDust, use where appropriate
+ Move+rename GetDustIndexes -> GetDust to replace HasDu
...

💬 hodlinator commented on pull request "policy: ephemeral dust followups":
(https://github.com/bitcoin/bitcoin/pull/31279#discussion_r1848428300)
nit: Could prefix with `out_` to communicate that they are out-reference-parameters instead of locals.

```suggestion
out_child_txid = tx->GetHash();
out_child_state.Invalid(TxValidationResult::TX_MEMPOOL_POLICY, "missing-ephemeral-spends",
```

💬 hodlinator commented on pull request "policy: ephemeral dust followups":
(https://github.com/bitcoin/bitcoin/pull/31279#discussion_r1848453460)
Thinking back to [discussion](https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1831104732) around how limiting prioritizetransaction for miners could backfire on the ecosystem in previous PR.

Encouraging miners to run with `-acceptnonstdtxn` means all *relayfee* limits for other transactions are effectively zero. But standardness touches upon other aspects of transactions as well (with more to be added in the future). So this is worse than only pushing miners to zero out *relayfee*-a
...

💬 hodlinator commented on pull request "policy: ephemeral dust followups":
(https://github.com/bitcoin/bitcoin/pull/31279#discussion_r1848433819)
perf nit: Could remove this line if using `.insert` and checking the return value instead of `.contains` above as suggested here https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1831063128.

The current version is slightly easier to read but does an *extra lookup*. I understand if you want to leave as-is for readability as the perf-hit is probably negligible.

💬 hodlinator commented on pull request "policy: ephemeral dust followups":
(https://github.com/bitcoin/bitcoin/pull/31279#discussion_r1849298256)
nit:
```suggestion
// Only MAX_DUST_OUTPUTS_PER_TX dust is permitted (on otherwise valid ephemeral dust)
```

💬 hodlinator commented on pull request "policy: ephemeral dust followups":
(https://github.com/bitcoin/bitcoin/pull/31279#discussion_r1849209897)
In 0f92fa2d468186a1526827a063f66e3042e028dc:
No longer used, should probably be removed there.

💬 hodlinator commented on pull request "policy: ephemeral dust followups":
(https://github.com/bitcoin/bitcoin/pull/31279#discussion_r1848737688)
In 60e18bb2dcd2c16a64e01023b793b0494cd6ca64:

nit: (Resolved in later commit) - `.value_or(null_txid)` seems unnecessary? Suggestion:
```C++
BOOST_CHECK_EQUAL(CheckEphemeralSpends({grandparent_tx_1, dust_non_spend, dust_spend}, minrelay, pool), dust_non_spend_txid);
```
Changing to that convention makes the introduction of `null_txid` pointless.

💬 hodlinator commented on pull request "policy: ephemeral dust followups":
(https://github.com/bitcoin/bitcoin/pull/31279#discussion_r1848750297)
Should further link `dust_index` with the value used by callers of the function, as current version has a hidden dependency.

<details>
<summary>
Sending as parameter
</summary>

```diff
diff --git a/src/test/txvalidation_tests.cpp b/src/test/txvalidation_tests.cpp
index b362abfa1d..0a51ff411f 100644
--- a/src/test/txvalidation_tests.cpp
+++ b/src/test/txvalidation_tests.cpp
@@ -91,8 +91,10 @@ static inline CTransactionRef make_tx(const std::vector<COutPoint>& inputs, int3
}

/
...

💬 hodlinator commented on pull request "policy: ephemeral dust followups":
(https://github.com/bitcoin/bitcoin/pull/31279#discussion_r1848502466)
nit: Could emplace movable `CTransaction`, apologies for not including it in my original suggestion.
```suggestion
txs.emplace_back([&] {
```

💬 hodlinator commented on pull request "policy: ephemeral dust followups":
(https://github.com/bitcoin/bitcoin/pull/31279#discussion_r1849177866)
nit: Could add `const` to `num_out` for symmetry with `num_in` as you are touching this line (pointed out in https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1832788903).