💬 stickies-v commented on pull request "Remove unsafe uint256S() and test-only uint160S()":
(https://github.com/bitcoin/bitcoin/pull/30773#discussion_r1740942391)
I had the same intuition, but kept them to be safe. Looking at it again, I agree with your view that they can be safely removed, so I've added a new commit 73a126f4f59470d839905d0eaaa26f689f7f2ba1 to do that and clean up the `arith_uint256S()` function entirely. Thanks!

💬 stickies-v commented on pull request "Remove unsafe uint256S() and test-only uint160S()":
(https://github.com/bitcoin/bitcoin/pull/30773#discussion_r1740942976)
Resolved by [removing the function altogether](https://github.com/bitcoin/bitcoin/commit/73a126f4f59470d839905d0eaaa26f689f7f2ba1).

💬 vasild commented on pull request "test: add end-to-end tests for CConnman and PeerManager":
(https://github.com/bitcoin/bitcoin/pull/26812#issuecomment-2324802375)
`e0bb306436...f5fe172e85`: rebase due to conflicts,

@jonatack there you go!

🤔 furszy reviewed a pull request: "rpc: add getdescriptoractivity"
(https://github.com/bitcoin/bitcoin/pull/30708#pullrequestreview-2275714079)
> How slow is this actually, though? Some kind of bounded cache would probably fix this pretty easily if it wound up taking non-negligible time relative to the other operations for these calls, which I would expect to swamp a non-IO computation like descriptor derivation.

That was also my initial expectation but then faced stuff like https://github.com/bitcoin/bitcoin/issues/30632#issuecomment-2310966130, which takes ~4 seconds to parse + derive. And it could get worse if the user adds some t
...

👍 fanquake approved a pull request: "doc: add `-DWITH_BDB=ON` to unix build docs"
(https://github.com/bitcoin/bitcoin/pull/30779#pullrequestreview-2275722547)
ACK ddef914bbb1e4fe87e8a85f17e4e839639fd97da

💬 andrewtoth commented on pull request "validation: write chainstate to disk every hour":
(https://github.com/bitcoin/bitcoin/pull/30611#discussion_r1740962468)
Well, changing the test to periodically flush after 55 minutes makes it a different test. That would mean the first flush is in the window and could randomly trigger a flush or not.
Was the failure after changing to `55min`?

💬 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.