💬 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

🚀 achow101 merged a pull request: "assumeutxo: Don't load a snapshot if it's not in the best header chain"
(https://github.com/bitcoin/bitcoin/pull/30320)

💬 achow101 commented on pull request "net: Allow -proxy=[::1] on nodes with IPV6 lo only":
(https://github.com/bitcoin/bitcoin/pull/30245#issuecomment-2237670866)
ACK 23333b7ed243071c9b4e4f04c727556d8065acbb

👍 hebasto approved a pull request: "depends: build FreeType with CMake"
(https://github.com/bitcoin/bitcoin/pull/29880#pullrequestreview-2186962592)
ACK ff4f3deb7b8adfcc90fb745440ce4be1176552ca, I've verified the actual compile options, they look sane.

🚀 achow101 merged a pull request: "net: Allow -proxy=[::1] on nodes with IPV6 lo only"
(https://github.com/bitcoin/bitcoin/pull/30245)

💬 fjahr commented on pull request "test, assumeutxo: Remove resolved todo comments and add new test":
(https://github.com/bitcoin/bitcoin/pull/30403#issuecomment-2237789775)
Rebased and since this is the last of the PRs addressing the TODOs comment in `feature_assumeutxo.py`, it now removes that whole section.

📝 theStack opened a pull request: "test: add creating/spending validity checks for rare output scripts"
(https://github.com/bitcoin/bitcoin/pull/30481)
This PR adds a functional test with a collection of output scripts that are "rare"/obscure, in the sense that they wouldn't be used or make sense within a regular payment transaction, but creating them is still considered standard, which is often considered surprising and counter-intuitive. They either
* use a weird pubkey encoding (-> hybrid pubkeys [in types P2PK, P2MS]),
* are provably unspendable (-> not-on-curve pubkeys [in types P2PK, P2MS, P2TR]), or
* are spendable by anyone with curr
...