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

💬 stickies-v commented on pull request "fix: Make TxidFromString() respect string_view length":
(https://github.com/bitcoin/bitcoin/pull/30436#discussion_r1682892102)
review note: it seems the `while` loop was replaced with `for` loop purely for readability, I don't see any functional change here.

💬 hebasto commented on pull request "qa: Functional test improvements":
(https://github.com/bitcoin/bitcoin/pull/30463#discussion_r1683503930)
> Ok, so it can be reproduced in Linux with hard links.

Right. But we do not use hard links on non-Windows systems.

> However, this leads back to my original question: Why not change the other places as well?

In other places, `os.path.realpath(__file__)` is used in conjunction with relative paths in the `test/functional/` directory. Soft links (on non-Windows systems) and hard links (on Windows) are created for every file in the `test/functional/` directory recursively. Therefore, the c
...

💬 brunoerg commented on pull request "wallet: Remove IsMine from migration code":
(https://github.com/bitcoin/bitcoin/pull/30328#discussion_r1683511942)
In 905e22b469a1e09df9ff0e98bf989a55642f301e "wallet: Remove IsMine from migration": Maybe it's worth updating the documentation?

```
// For every script in mapScript, only the ISMINE_SPENDABLE ones are being tracked.
```

💬 achow101 commented on pull request "assumeutxo: Don't load a snapshot if it's not in the best header chain":
(https://github.com/bitcoin/bitcoin/pull/30320#issuecomment-2237639016)
ACK 55b6d7be68a6f6c3882588ffd5b9349d885ed953