💬 l0rinc commented on pull request "Remove unsafe uint256S() and test-only uint160S()":
(https://github.com/bitcoin/bitcoin/pull/30773#discussion_r1741088168)
> Have a look at the UniValue::get_str() implementation

I already checked all of that, that's why I was asking if we'd rather they throw on the call site. You've already answered that you think it's orthogonal.

> I am generally in favour of pushing UniValue user input parsing into stricter types further back to the edges, though.

Yes, that's what I was saying basically - though I needed 2 rounds to understand it myself why this was bothering me.

👍 l0rinc approved a pull request: "Remove unsafe uint256S() and test-only uint160S()"
(https://github.com/bitcoin/bitcoin/pull/30773#pullrequestreview-2275917562)
Minor nit suggestions, ACK a9d19ae1aa06f14c1892bb4e37c3f023bc61b39b

💬 l0rinc commented on pull request "Remove unsafe uint256S() and test-only uint160S()":
(https://github.com/bitcoin/bitcoin/pull/30773#discussion_r1741081872)
since we've already included `setup_common` which contains `std::ostream& operator<<(std::ostream& os, const arith_uint256& num)` we might as well use `BOOST_CHECK_EQUAL` here:
```diff
diff --git a/src/test/arith_uint256_tests.cpp b/src/test/arith_uint256_tests.cpp
index a434a1d401..9a6307c1bd 100644
--- a/src/test/arith_uint256_tests.cpp
+++ b/src/test/arith_uint256_tests.cpp
@@ -64,51 +64,50 @@ static std::string ArrayToString(const unsigned char A[], unsigned int width)

BOOST_AUTO_
...

💬 l0rinc commented on pull request "validation: write chainstate to disk every hour":
(https://github.com/bitcoin/bitcoin/pull/30611#discussion_r1741091951)
we don't have to add a variable, we can just inline it to:
```C++
if (fDoFullFlush || m_next_write == NodeClock::time_point::max()) {
```

Which would simplify a lot (logically, making it obvious when we're queuing the next run and also codewise, reuding duplication)

💬 l0rinc commented on pull request "validation: write chainstate to disk every hour":
(https://github.com/bitcoin/bitcoin/pull/30611#discussion_r1741092396)
Mostly because I ran it and it behaved in a weird way (sometimes passing) so I have a hard time trusting it now. It never passed for ALL of these values, so at least they're more reliable.

💬 fjahr commented on pull request "Embed default ASMap as binary dump header file":
(https://github.com/bitcoin/bitcoin/pull/28792#discussion_r1741093949)
done

💬 fjahr commented on pull request "Embed default ASMap as binary dump header file":
(https://github.com/bitcoin/bitcoin/pull/28792#issuecomment-2325028945)
Switched back to a native approach in `cmake` without `python` or `xxd`.

💬 l0rinc commented on pull request "validation: write chainstate to disk every hour":
(https://github.com/bitcoin/bitcoin/pull/30611#discussion_r1741105265)
now that we have two clocks here, this doesn't seem to work anymore (this seems to be the build failure's reason). Do we really need the `- nNow` part?
```suggestion
int64_t{Ticks<std::chrono::microseconds>(SteadyClock::now())},
```
or if we do:
```suggestion
int64_t{Ticks<std::chrono::microseconds>(SteadyClock::now()) - Ticks<std::chrono::microseconds>(nNow)},
```

💬 stickies-v commented on pull request "Remove unsafe uint256S() and test-only uint160S()":
(https://github.com/bitcoin/bitcoin/pull/30773#discussion_r1741105267)
I'm not touching any of those lines in this PR so I'd rather keep this PR a bit narrower.

```diff
- BOOST_CHECK_EQUAL((R1L & arith_uint256{0xffffffffffffffff}), arith_uint256{R1LLow64});
+ BOOST_CHECK_EQUAL(R1L & arith_uint256{0xffffffffffffffff}, arith_uint256{R1LLow64});
```

Thanks, I'll patch this if I have to force-push.

(note: you can hide long code blocks with a [`<details>...</details>` tag](https://docs.github.com/en/get-started/writing-on-github/working-with-advanced-f
...

💬 l0rinc commented on pull request "validation: write chainstate to disk every hour":
(https://github.com/bitcoin/bitcoin/pull/30611#discussion_r1741107409)
Using SteadyClock in https://github.com/bitcoin/bitcoin/pull/30611#discussion_r1741105265 seems to have stabilized the tests.
Approach ACK

💬 l0rinc commented on pull request "validation: write chainstate to disk every hour":
(https://github.com/bitcoin/bitcoin/pull/30611#discussion_r1741109081)
if you decide to keep this structure instead of the exhaustive one:
```suggestion
// Time to periodically flush is between 50 and 70 minutes (inclusive)
```

📝 hebasto converted_to_draft a pull request: "build: Use correct variable name"
(https://github.com/bitcoin/bitcoin/pull/30791)
This was overlooked after https://github.com/bitcoin-core/secp256k1/pull/1546.

💬 l0rinc commented on pull request "validation: write chainstate to disk every hour":
(https://github.com/bitcoin/bitcoin/pull/30611#discussion_r1741110745)
if you decide to keep this structure, please add boundary test for `50min` turning `m_did_flush` to true.
Would also cover `+ 71min` for the same reason.

💬 andrewtoth commented on pull request "validation: write chainstate to disk every hour":
(https://github.com/bitcoin/bitcoin/pull/30611#discussion_r1741111508)
I don't think it would, because it's random.

💬 maflcko commented on 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#issuecomment-2325061180)
> Regtest

Haven't followed any of this in detail, but on regtest, modifying the consensus rule on the command line should be harmless and is already being done to closer mirror main-net, where the defaults don't achieve it. It should be easy to apply in this case as well, if there is need to.

💬 l0rinc commented on pull request "refactor: Extend CScript with `operator<<` for `std::array`":
(https://github.com/bitcoin/bitcoin/pull/30765#discussion_r1741115644)
I'll investigate these tomorrow, thanks for the comments!

💬 l0rinc commented on pull request "Remove unsafe uint256S() and test-only uint160S()":
(https://github.com/bitcoin/bitcoin/pull/30773#discussion_r1741116936)
> you can hide long code blocks

Ah, I don't see them anymore, `Refined Github` extension does it automatically. I'll do it next time.

💬 vasild commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#issuecomment-2325087575)
`31d841ddb8...08c9a8254a`: rebase to pick CMake

💬 ryanofsky commented on pull request "refactor: Extend CScript with `operator<<` for `std::array`":
(https://github.com/bitcoin/bitcoin/pull/30765#discussion_r1741129350)
> Would it make sense to drop operator<< from CScript, and instead have explicit "Append" and "Push" member functions (that take spans), and do the expected thing?

I think it makes sense, especially because cscripts are serializable, so it is confusing that `x << script` does an entirely different operation than `script << x`. But making this change would expand the scope of the PR beyond what it is currently doing which is just making cscript work a little better with hex literals from #3037
...

💬 glozow commented on pull request "cluster mempool: optimized candidate search":
(https://github.com/bitcoin/bitcoin/pull/30286#issuecomment-2325113562)
reACK 100e1b9ecb2 via range-diff