💬 andrewtoth commented on pull request "validation: write chainstate to disk every hour":
(https://github.com/bitcoin/bitcoin/pull/30611#discussion_r1740966394)
Why would we need to test every value in the window? Testing the edges of the window accomplishes the same thing, no?

💬 andrewtoth commented on pull request "validation: write chainstate to disk every hour":
(https://github.com/bitcoin/bitcoin/pull/30611#discussion_r1740969969)
I'm not sure this is an improvement. We would be adding a new variable, instead of calling a function twice. Seems like a wash to me.

🚀 fanquake merged a pull request: "doc: add `-DWITH_BDB=ON` to unix build docs"
(https://github.com/bitcoin/bitcoin/pull/30779)

💬 tdb3 commented on pull request "test: Avoid intermittent timeout in p2p_headers_sync_with_minchainwork.py":
(https://github.com/bitcoin/bitcoin/pull/30761#discussion_r1740973996)
That's reasonable. I don't feel super strongly about it.

👍 tdb3 approved a pull request: "test: Avoid intermittent timeout in p2p_headers_sync_with_minchainwork.py"
(https://github.com/bitcoin/bitcoin/pull/30761#pullrequestreview-2275746016)
ACK fa247e6e8c7fddf9e3461c3e2e6f5fade0fe64cf

🤔 mzumsande reviewed a pull request: "Have miner account for timewarp mitigation, activate on regtest, lower nPowTargetTimespan to 144 and add test"
(https://github.com/bitcoin/bitcoin/pull/30681#pullrequestreview-2275751672)
I'm late and only saw this PR in the release notes, but FWIW I think that changing regtest consensus rules in order to add test coverage for testnet is setting the wrong priority.

In my opinion, testnet 4 isn't anywhere near important enough to justify that. Regtest, in particular unit and functional tests, primarily exists to test mainnet and should therefore follow it as close as possible. Rather than this, we should add tests to regtest that demonstrate that `timewarp` is _not_ mitigated
...

🚀 fanquake merged a pull request: "test: Avoid intermittent timeout in p2p_headers_sync_with_minchainwork.py"
(https://github.com/bitcoin/bitcoin/pull/30761)

💬 andrewtoth commented on pull request "validation: write chainstate to disk every hour":
(https://github.com/bitcoin/bitcoin/pull/30611#discussion_r1740986718)
Ahh, I see what you mean. We need to trigger an initial flush to set `m_next_write` first before testing the periodic flush window. Fixed.

🤔 marcofleon reviewed a pull request: "test: Avoid intermittent timeout in p2p_headers_sync_with_minchainwork.py"
(https://github.com/bitcoin/bitcoin/pull/30761#pullrequestreview-2275767545)
Code review ACK fa247e6e8c7fddf9e3461c3e2e6f5fade0fe64cf

💬 fjahr commented on pull request "Embed default ASMap as binary dump header file":
(https://github.com/bitcoin/bitcoin/pull/28792#issuecomment-2324869760)
> To clarify: I am against including the data twice. 1.6 MB on every bump is bad enough. Twice that for no strong reason seems annoying to anyone cloning the repo in the future.

Now I'm confused, the header file isn't included anymore since a while ago so the data is included only once already, or am I missing something? Are you just saying that shouldn't be changed back?

💬 l0rinc commented on pull request "fuzz: speed up addrman":
(https://github.com/bitcoin/bitcoin/pull/30688#discussion_r1740998751)
I meant generating the size directly to be able to allocate, i.e.
```C++
auto count = fuzzed_data_provider.ConsumeIntegralInRange<size_t>(0, 1000);
std::vector<CAddress> addresses;
addresses.reserve(count);
for (size_t i = 0; i < count; ++i) {
addresses.push_back(ConsumeAddress(fuzzed_data_provider));
}
```
like we seem to be doing in [many](https://github.com/bitcoin/bitcoin/blob/master/src/test/fuzz/bitdeque.cpp#L73) [other](https://github.com/bitcoin/bitcoin/blob/master/src/test/
...

💬 andrewtoth commented on pull request "validation: write chainstate to disk every hour":
(https://github.com/bitcoin/bitcoin/pull/30611#discussion_r1740999482)
Done.

💬 andrewtoth commented on pull request "validation: write chainstate to disk every hour":
(https://github.com/bitcoin/bitcoin/pull/30611#discussion_r1740999675)
Done.

💬 l0rinc commented on pull request "test: cover base32/base58/base64 with symmetric roundtrip fuzz (and padding) tests":
(https://github.com/bitcoin/bitcoin/pull/30746#issuecomment-2324882761)
> it seems odd that the fuzz tests would time out here

Split the big fuzz target into separate base58_encode_decode/base58check_encode_decode/base32_encode_decode/base64_encode_decode/psbt_base64_decode - it's cleaner like this anyway.

📝 theStack opened a pull request: "scripted-diff: adapt binary paths in docs (`./src/...` -> `./build/src/...`)"
(https://github.com/bitcoin/bitcoin/pull/30789)
This is a simple quick-fix to make the docs match the new directory structure after the switch to CMake (see #30454 and #30664). On the long term, it may make sense to improve some of these docs by introducing and using environment variables like `BITCOIN_CLI` and `BITCOIND` instead of specifying the path repeatedly.

💬 fanquake commented on pull request "scripted-diff: adapt binary paths in docs (`./src/...` -> `./build/src/...`)":
(https://github.com/bitcoin/bitcoin/pull/30789#issuecomment-2324898009)
Note this is also included in #30741.

💬 l0rinc commented on pull request "refactor: Extend CScript with `operator<<` for `std::array`":
(https://github.com/bitcoin/bitcoin/pull/30765#discussion_r1741014685)
> will this be touched again?

Not sure, isn't that the case with vector as well?

> so it just seems like right now this is adding more code while leaving the underlying issue.

Can you please detail why you think this is the case? `_hex_v_u8` was mostly eliminated and the production code is basically the same (+3 lines for the other overload).

> Maybe this could be a span or a range?

I understood we wanted to avoid doing that in https://github.com/bitcoin/bitcoin/pull/30377#discus
...

💬 stickies-v commented on pull request "Remove unsafe uint256S() and test-only uint160S()":
(https://github.com/bitcoin/bitcoin/pull/30773#issuecomment-2324906247)
And force-pushed again to rebase on top of #30377 to avoid silent merge conflicts e.g. because of the now lowercase-only `uint256` hex string constructor.

✅ theStack closed a pull request: "scripted-diff: adapt binary paths in docs (`./src/...` -> `./build/src/...`)"
(https://github.com/bitcoin/bitcoin/pull/30789)

💬 theStack commented on pull request "scripted-diff: adapt binary paths in docs (`./src/...` -> `./build/src/...`)":
(https://github.com/bitcoin/bitcoin/pull/30789#issuecomment-2324907676)
> Note this is also included in #30741.

Oh, I missed that one, it seems to be more thorough anyways (e.g. also tackling .bt files). Closing in favor of #30741.