💬 dergoegge commented on pull request "fuzz: Version handshake":
(https://github.com/bitcoin/bitcoin/pull/30413#discussion_r1683190037)
Measuring exec/s without lazy init:


With:


💬 dergoegge commented on pull request "fuzz: Version handshake":
(https://github.com/bitcoin/bitcoin/pull/30413#discussion_r1683143984)
There are more message types that are allowed after the version message and before the verack (some of them are also allowed post verack), essentially all of the ones processed in [this block](https://github.com/bitcoin/bitcoin/blob/20ccb30b7af1c30cece2b0579ba88aa101583f07/src/net_processing.cpp#L3894-L4061) (`wtxidrelay`, `sendaddrv2`, `sendtxrcncl`, `sendheaders`, `sendcmpct`).

I used `ALL_NET_MESSAGE_TYPES`, so that we can avoid touching this test if we add more handshake related message t
...

💬 dergoegge commented on pull request "fuzz: Version handshake":
(https://github.com/bitcoin/bitcoin/pull/30413#discussion_r1683123261)
My idea was that this might help a fuzzer to more realistically simulate how time would change between messages. Setting it directly would of course also work but I thought it might result in more mutations that are completely bogus (the time isn't attacker controlled so this seemed less useful). Happy to change it, probably doesn't make a huge difference. What do you think?

💬 Sjors commented on pull request "Introduce waitFeesChanged() mining interface":
(https://github.com/bitcoin/bitcoin/pull/30443#issuecomment-2237077920)
Rebased after #30356 landed. Changed `fees_before` to be a reference to `CAmount` based on the above experience of using it.

💬 Sjors commented on pull request "Introduce waitTipChanged() mining interface, replace RPCNotifyBlockChange and drop CRPCSignals":
(https://github.com/bitcoin/bitcoin/pull/30409#issuecomment-2237085603)
Rebased after #30356 (`#include` order).

💬 Sjors commented on pull request "guix: bump time-machine to e6e70abe65850da0ae69428654375d367088ef5e":
(https://github.com/bitcoin/bitcoin/pull/30452#issuecomment-2237090936)
guix hashes:

```
365b4e5e4f876a949834d6f444edb079851e7e77b4fc77f7d6bafd1958f87a10 guix-build-314407a2fb80/output/aarch64-linux-gnu/SHA256SUMS.part
d8f7f8a9d84e37df272a53e5a69f4325196dfc7d43b1a2c12415e81a140f80ad guix-build-314407a2fb80/output/aarch64-linux-gnu/bitcoin-314407a2fb80-aarch64-linux-gnu-debug.tar.gz
aaafb96f75597be1206329b5632e32d1f844e15e69415184e8aada3397789bc6 guix-build-314407a2fb80/output/aarch64-linux-gnu/bitcoin-314407a2fb80-aarch64-linux-gnu.tar.gz
d3eec1297168c6104
...

💬 Sjors commented on pull request "Have createNewBlock() return a BlockTemplate interface":
(https://github.com/bitcoin/bitcoin/pull/30440#issuecomment-2237092200)
Rebased after https://github.com/bitcoin/bitcoin/pull/30356 landed.

💬 theuni commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1683210749)
There are a few places here that seem to be changing behavior from setting flags to adding flags. Are the outcomes guaranteed to be the same?

👍 maflcko approved a pull request: "Early logging improvements"
(https://github.com/bitcoin/bitcoin/pull/30386#pullrequestreview-2185698999)
ACK f3632d53a5be7efe61be8181ff481d30f7313bd4 🚲

<details><summary>Show signature</summary>

Signature:

```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: ACK f3632d53a5be7efe61be8181ff
...

💬 maflcko commented on pull request "Early logging improvements":
(https://github.com/bitcoin/bitcoin/pull/30386#discussion_r1682767124)
q in 0b1960f1b29cfe5209ac68102c8643fc9553f247: Any reason to assert this but not the that the other `m_print_to_*` fields are false? What is the difference of a developer writing `PushBackCallback(...); DisableLogging()` to crash the program vs a developer writing `m_print_to_console=true;DisableLogging()` to achieve the same?

💬 maflcko commented on pull request "Early logging improvements":
(https://github.com/bitcoin/bitcoin/pull/30386#discussion_r1683194609)
nit in a67baea866719837865864a5eb56a47a9f132de1: Could switch the two lines and use `std::move`?

💬 maflcko commented on pull request "Early logging improvements":
(https://github.com/bitcoin/bitcoin/pull/30386#discussion_r1683186853)
q in a67baea866719837865864a5eb56a47a9f132de1: Why use `str` here? Seems odd to possibly leak terminal control codes into the log?

💬 maflcko commented on pull request "Early logging improvements":
(https://github.com/bitcoin/bitcoin/pull/30386#discussion_r1682785077)
style nit in 3854484b988dfc7144e3ed3ca09de563e435cc34:

`1'000'000`

💬 maflcko commented on pull request "fix: Make TxidFromString() respect string_view length":
(https://github.com/bitcoin/bitcoin/pull/30436#discussion_r1683221641)
> I'm surprised looking at the implementation of SetHex to see how unreliable it is in general. Since it can't report errors it winds up just ignoring unexpected input:

Yes, those are known shortcomings. The function will be removed completely in the future, by this will be done in a follow-up.

💬 maflcko commented on pull request "fix: Make TxidFromString() respect string_view length":
(https://github.com/bitcoin/bitcoin/pull/30436#discussion_r1683226283)
> It seems not great

I don't think this is a change in behavior (previously this was the behavior and afterward this remains the behavior), and the function will be removed completely in the future anyway, so I am not sure how much time should be spent on playing code-golf here.

Happy to re-ACK a trivial fixup, but if it takes more than a few seconds to review it is probably not worth it.

💬 instagibbs commented on pull request "Fee Estimation: change `estimatesmartfee` default mode to `economical`":
(https://github.com/bitcoin/bitcoin/pull/30275#issuecomment-2237123724)
> Could you please clarify about the test what specific regressions you're concerned about? Thanks!

A boolean flip where the intended behavior isn't followed and we are giving conservative again?

💬 sipa commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1683242500)
Yeah, if you load a few lines above each of those instances it'll be clear it's always due to newly inserted entries.

💬 ryanofsky commented on pull request "fix: Make TxidFromString() respect string_view length":
(https://github.com/bitcoin/bitcoin/pull/30436#discussion_r1683245538)
Thanks, I did not know this function will be removed. It would be helpful to mention in the PR description that it will be removed, so reviewers can know how worried to be about the quality of the code.

💬 maflcko commented on pull request "fix: Make TxidFromString() respect string_view length":
(https://github.com/bitcoin/bitcoin/pull/30436#discussion_r1683252772)
> Thanks, I did not know this function will be removed. It would be helpful to mention in the PR description that it will be removed, so reviewers can know how worried to be about the quality of the code.

Well, it will be removed, given that someone writes the code and reviews it. A tracking issue can be created, if you want, but I think the pull request description is the wrong place for this tracking stuff. As for reviewers: I'd say reviewers should worry the most about any regression or wo
...

💬 hebasto commented on pull request "Fix SSE4.1-related issues":
(https://github.com/bitcoin/bitcoin/pull/28893#issuecomment-2237154433)
Ported to the CMake-based build system in https://github.com/hebasto/bitcoin/pull/268.