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

💬 luisschwab commented on issue "`scantxoutset` should output the block hash instead of block height":
(https://github.com/bitcoin/bitcoin/issues/30478#issuecomment-2237347759)
Sure, should also be trivial to add.

📝 Psc544 opened a pull request: "6fe04302f75958ca1ad398e637e9e44c680d3a2eCreate devcontainer.json"
(https://github.com/bitcoin/bitcoin/pull/30480)
<!--
*** Please remove the following help text before submitting: ***

Pull requests without a rationale and clear improvement may be closed
immediately.

GUI-related pull requests should be opened against
https://github.com/bitcoin-core/gui
first. See CONTRIBUTING.md
-->

<!--
Please provide clear motivation for your patch and explain how it improves
Bitcoin Core user experience or Bitcoin Core developer experience
significantly:

* Any test improvements or new tests that improv
...

✅ fanquake closed a pull request: "6fe04302f75958ca1ad398e637e9e44c680d3a2eCreate devcontainer.json"
(https://github.com/bitcoin/bitcoin/pull/30480)

📝 fanquake locked a pull request: "6fe04302f75958ca1ad398e637e9e44c680d3a2eCreate devcontainer.json"
(https://github.com/bitcoin/bitcoin/pull/30480)
<!--
*** Please remove the following help text before submitting: ***

Pull requests without a rationale and clear improvement may be closed
immediately.

GUI-related pull requests should be opened against
https://github.com/bitcoin-core/gui
first. See CONTRIBUTING.md
-->

<!--
Please provide clear motivation for your patch and explain how it improves
Bitcoin Core user experience or Bitcoin Core developer experience
significantly:

* Any test improvements or new tests that improv
...

👍 ryanofsky approved a pull request: "Have createNewBlock() return a BlockTemplate interface"
(https://github.com/bitcoin/bitcoin/pull/30440#pullrequestreview-2186655119)
Light code review ACK 52c80dff8e26c359d13c4b33e3c75a8519cdcee7, since I still didn't look very closely at merkle path function yet

💬 ryanofsky commented on pull request "Have createNewBlock() return a BlockTemplate interface":
(https://github.com/bitcoin/bitcoin/pull/30440#discussion_r1683349167)
In commit "Have createNewBlock return BlockTemplate interface" (6536b345a346670b8733e5372f988452727a2554)

I don't see any way this condition could be true, now or previously. Would suggest changing this to assert(block_template) or CHECK_NONFATAL(block_template)

Same comment also applies to createNewBlock calls below on lines 374 and 817.

The check [here](https://github.com/bitcoin/bitcoin/blob/20ccb30b7af1c30cece2b0579ba88aa101583f07/src/node/miner.cpp#L108-L111) also appears to be dea
...

💬 ryanofsky commented on pull request "Have createNewBlock() return a BlockTemplate interface":
(https://github.com/bitcoin/bitcoin/pull/30440#discussion_r1683353989)
In commit "Have createNewBlock return BlockTemplate interface" (6536b345a346670b8733e5372f988452727a2554)

Would be good to guard against undefined behavior by adding `assert(m_block_template);` in the constructor body and declaring `m_block_template` as `const unique_ptr` to prevent it being reset to null.

If this is done, the `Assert` in `getBlockHeader` could also be removed so it is more consistent with the other methods.

💬 hebasto commented on pull request "build: Introduce CMake-based buid system":
(https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1683411958)
Fixed in https://github.com/hebasto/bitcoin/pull/270.

💬 sipa commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1683431461)
It makes sense to add a few Prev() calls to the unit test too. Now or later.

💬 achow101 commented on pull request "rest: Reject negative outpoint index early in getutxos parsing":
(https://github.com/bitcoin/bitcoin/pull/30444#issuecomment-2237490872)
ACK fac932bf93d9bd8cb7bef93f04ecf48ea2ccf1d1

🤔 sipa reviewed a pull request: "Don't empty dbcache on prune flushes: >30% faster IBD"
(https://github.com/bitcoin/bitcoin/pull/28280#pullrequestreview-2186784562)
ACK d7f94e0fa9ec3641405e6909ab7da4e48865e840

💬 andrewtoth commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1683449818)
I can add Prev tests to the coinscachepair_tests and SanityCheck to the coins_tests in a follow-up.