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

💬 mzumsande commented on issue "Assertion `setBlockIndexCandidates.count(pindex)' failed":
(https://github.com/bitcoin/bitcoin/issues/16444#issuecomment-2237175337)
While #16849 fixed inconsistencies in `invalidateblock`, failures continued because similar inconsistencies can also happen during reconsiderblock. See #30479 for an explanation and a fix.

💬 brunoerg commented on pull request "wallet: Remove IsMine from migration code":
(https://github.com/bitcoin/bitcoin/pull/30328#discussion_r1683274753)
```suggestion
// We don't care about these types because they are not spendable
```

💬 hebasto commented on pull request "build: Introduce CMake-based buid system":
(https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1683279748)
Please see https://github.com/hebasto/bitcoin/pull/269.

💬 ryanofsky commented on pull request "kernel, refactor: return error status on all fatal errors":
(https://github.com/bitcoin/bitcoin/pull/29700#issuecomment-2237212741)
Rebased ad1e73590d102edf3738986c1382b5e36a0ed727 -> e04e259146238f9a733edf3fd7b192db95f66e71 ([`pr/fatalresult.18`](https://github.com/ryanofsky/bitcoin/commits/pr/fatalresult.18) -> [`pr/fatalresult.19`](https://github.com/ryanofsky/bitcoin/commits/pr/fatalresult.19), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/fatalresult.18-rebase..pr/fatalresult.19)) to fix conflicts with #29668 and #30428

💬 luisschwab commented on issue "`scantxoutset` should output the block hash instead of block height":
(https://github.com/bitcoin/bitcoin/issues/30478#issuecomment-2237247765)
> So I don't think there is a strong benefit in removing it?

Yes, I agree.

🤔 ryanofsky reviewed a pull request: "refactor: Improve assumeutxo state representation"
(https://github.com/bitcoin/bitcoin/pull/30214#pullrequestreview-2186500256)
re: https://github.com/bitcoin/bitcoin/pull/30214#pullrequestreview-2177291599

Thanks for reviewing! Will rebase and try to address comments. I'm especially interested if you have ideas on splitting up documentation and code changes into separate PRs.

> I am not so deep into the current state of the kernel work though, so I can not comment on that part, for example the newly introduced typing file. I guess @TheCharlatan could give his nod to that here.

Sure, for background kernel/types.
...

💬 ryanofsky commented on pull request "refactor: Improve assumeutxo state representation":
(https://github.com/bitcoin/bitcoin/pull/30214#discussion_r1683253240)
re: https://github.com/bitcoin/bitcoin/pull/30214#discussion_r1677573399

> nit: In other places similar checks are inlined without a helper method. That might be actually a bit more clear as well.

Good point, at least at first glance this looks reasonable to inline.

💬 ryanofsky commented on pull request "refactor: Improve assumeutxo state representation":
(https://github.com/bitcoin/bitcoin/pull/30214#discussion_r1683252990)
re: https://github.com/bitcoin/bitcoin/pull/30214#discussion_r1677640152

> Not sure why we need this when in another commit we remove `GetAll()` and instead use `m_chainstates`. Seems kind of inconsistent but maybe I am missing something.

Is this comment about the `Validity()` method? This is just supposed to be a public accessor for code that cannot access the private m_validity member/

💬 ryanofsky commented on pull request "refactor: Improve assumeutxo state representation":
(https://github.com/bitcoin/bitcoin/pull/30214#discussion_r1683252537)
> Why do you add this function around here instead of using `m_chainstates` as well?

I need to double-check but IIRC, the issue is that m_chainstates requires a lock to access, and it would require larger changes and more verbosity in the tests if they had to be changed to access m_chainstates directly. Will follow up and add a comment describing purpose of this function.

💬 maflcko commented on issue "`scantxoutset` should output the block hash instead of block height":
(https://github.com/bitcoin/bitcoin/issues/30478#issuecomment-2237344411)
While touching this, another thing to add could be the number of confirmations? I understand that this wouldn't help machine consumers of the interface, but human callers may find it useful?

💬 instagibbs commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#issuecomment-2237345861)
@petertodd I'll give a non-TRUC version another try and report back. I may have been over-thinking things.