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

💬 ryanofsky commented on pull request "multiprocess: add bitcoin-mine test program":
(https://github.com/bitcoin/bitcoin/pull/30437#issuecomment-2237537095)
Rebased bdd68d5bca483071ca0729e6f0d2d71706ad21e7 -> fba04d7756c77ed9371c5dc90d3bebc9aa767f4b ([`pr/mine.6`](https://github.com/ryanofsky/bitcoin/commits/pr/mine.6) -> [`pr/mine.7`](https://github.com/ryanofsky/bitcoin/commits/pr/mine.7), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/mine.6-rebase..pr/mine.7)) due to conflict with #30356

💬 brunoerg commented on pull request "wallet: Remove IsMine from migration code":
(https://github.com/bitcoin/bitcoin/pull/30328#discussion_r1683481047)
In 905e22b469a1e09df9ff0e98bf989a55642f301e "wallet: Remove IsMine from migration": Just a question, but could it be a vector of `CPubKey` then use `IsCompressed`?

🚀 achow101 merged a pull request: "rest: Reject negative outpoint index early in getutxos parsing"
(https://github.com/bitcoin/bitcoin/pull/30444)

👍 stickies-v approved a pull request: "fix: Make TxidFromString() respect string_view length"
(https://github.com/bitcoin/bitcoin/pull/30436#pullrequestreview-2185910679)
ACK c3a9c71c7077324bf0aa40f834f7537a14619340,

> are lacking a good description that actually says what the bug is.

I agree, I spent quite a bit of time getting to the bottom of this and more documentation would have sped that up

> but internally it is calling the uint256S function which expects a nul-terminated string

Maybe I'm being too pedantic (or wrong), but I'm not sure this is 100% correct? I don't think the `uint256S(std::string_view str)` `str` needs to be null-terminated. Ra
...