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

💬 fanquake commented on pull request "depends: build zeromq with CMake":
(https://github.com/bitcoin/bitcoin/pull/29723#issuecomment-2237161800)
> is non-deterministic across architectures.

Should be fixed now.

👍 theuni approved a pull request: "depends: build FreeType with CMake"
(https://github.com/bitcoin/bitcoin/pull/29880#pullrequestreview-2186515247)
ACK ff4f3deb7b8adfcc90fb745440ce4be1176552ca

Guix build (x86_64):
```
12c161c6b94ccb15d420fe27f7a812c95247a2614db5f8c6e666e04b5ef0a7d3 aarch64-linux-gnu/bitcoin-ff4f3deb7b8a-aarch64-linux-gnu-debug.tar.gz
99c6d258715f38c7b56eefd0b08e80552012e8903f26d28225b690f432839545 aarch64-linux-gnu/bitcoin-ff4f3deb7b8a-aarch64-linux-gnu.tar.gz
bd6c916cc5fdd322bf9e7e3cecbe45a68551c0135d63b35709b32fccde289834 aarch64-linux-gnu/SHA256SUMS.part
18d7fb6f45b585a4313d115ac95f049dfc27b66857571933a3f5851f
...

📝 mzumsande opened a pull request: "validation: Add eligible ancestors of reconsidered block to setBlockIndexCandidates"
(https://github.com/bitcoin/bitcoin/pull/30479)
When we call `reconsiderblock` for some block,  `Chainstate::ResetBlockFailureFlags` puts the descendants of that block into `setBlockIndexCandidates` (if they meet the criteria, i.e. have more work than the tip etc.), but never put any ancestors into the set even though we do clear their failure flags.

I think that this is wrong, because `setBlockIndexCandidates` should always contain all eligible indexes that have at least as much work as the current tip, which can include ancestors of the
...