💬 ryanofsky commented on pull request "blockstorage: Separate reindexing from saving new blocks":
(https://github.com/bitcoin/bitcoin/pull/29975#discussion_r1598626879)
In commit "doc: Improve doc for functions involved in saving blocks to disk" (f90098e57dd8dc77a9788f5af7a529b32ca37df6)

I think it is confusing that this says "during reindex, only the statistics are updated" instead of "If the block is already stored, only the statistics are updated." It is true that this function is only called with already-stored blocks during reindexing, but that doesn't seem obvious, and you wouldn't know it without checking every code path that calls this function.

W
...

👍 ryanofsky approved a pull request: "blockstorage: Separate reindexing from saving new blocks"
(https://github.com/bitcoin/bitcoin/pull/29975#pullrequestreview-2052927360)
Code review ACK 9cf475ffffb869cd55c2b2f3be84d7c90b199521. I left some suggestions to clean up comments in intermediate commits, but everything looks good in the end.

💬 ryanofsky commented on pull request "blockstorage: Separate reindexing from saving new blocks":
(https://github.com/bitcoin/bitcoin/pull/29975#discussion_r1598604063)
In commit "doc: Improve doc for functions involved in saving blocks to disk" (f90098e57dd8dc77a9788f5af7a529b32ca37df6)

This comment is wrong at this point in the PR. The way FindBlockPos works right now, you only pass it the size of the serialized CBlock plus the size of the separator fields when fKnown is false. When fKnown is true, you are supposed to pass it just the size of serialized CBlock, without the size of the separator fields.

💬 ryanofsky commented on pull request "blockstorage: Separate reindexing from saving new blocks":
(https://github.com/bitcoin/bitcoin/pull/29975#discussion_r1598649938)
re: https://github.com/bitcoin/bitcoin/pull/29975#discussion_r1597601896

> does `[[nodiscard]]` still make sense now?

I think it does make sense, since the function can still fail and checking the return value is the only way to know.

💬 ryanofsky commented on pull request "blockstorage: Separate reindexing from saving new blocks":
(https://github.com/bitcoin/bitcoin/pull/29975#discussion_r1598677559)
In commit "blockstorage: Rename FindBlockPos and have it return a FlatFilePos" (d5904bd250f41b935d6ec776373d05b42d71b04f)

Seems like it would be good to add this comment in earlier commit "doc: Improve doc for functions involved in saving blocks to disk" (f90098e57dd8dc77a9788f5af7a529b32ca37df6), since the information does apply there, so the comment is not changing unnecessarily here.

👍 theuni approved a pull request: "rpc: move UniValue in blockToJSON"
(https://github.com/bitcoin/bitcoin/pull/30094#pullrequestreview-2053082236)
utACK b77bad309e92f176f340598eec056eb7bff86f5f

💬 josibake commented on pull request "wallet: fix unrelated parent conflict doesn't cause child tx to be marked as conflict":
(https://github.com/bitcoin/bitcoin/pull/29680#issuecomment-2108083971)
The benefits I see are we can keep the existing Enum and code for converting the Enum to a string. This would also allow updating the function signatures with a scripted-diff, which makes larger refactors like this easier to verify as a reviewer.

It also feels like a more natural fit to me and my gut feeling is that having a class to encapsulate the removal reasons and necessary data will be more maintainable and extensible going forward.

💬 hebasto commented on pull request "[DO NOT MERGE] cmake: Migrate CI scripts to CMake-based build system -- WIP":
(https://github.com/bitcoin/bitcoin/pull/29790#discussion_r1598708672)
Thanks! Removed.

💬 instagibbs commented on pull request "Cluster size 2 package rbf":
(https://github.com/bitcoin/bitcoin/pull/28984#discussion_r1598710812)
was wondering about this case, added with some modifications for test simplicity

💬 instagibbs commented on pull request "Cluster size 2 package rbf":
(https://github.com/bitcoin/bitcoin/pull/28984#discussion_r1598710933)
done

💬 instagibbs commented on pull request "Cluster size 2 package rbf":
(https://github.com/bitcoin/bitcoin/pull/28984#discussion_r1598711002)
I created `DEFAULT_CHILD_FEE` and used that pretty much everywhere it made sense

💬 instagibbs commented on pull request "Cluster size 2 package rbf":
(https://github.com/bitcoin/bitcoin/pull/28984#discussion_r1598711061)
taken

💬 instagibbs commented on pull request "Cluster size 2 package rbf":
(https://github.com/bitcoin/bitcoin/pull/28984#discussion_r1598711106)
done

💬 Sjors commented on pull request "depends: set AR & RANLIB for CMake":
(https://github.com/bitcoin/bitcoin/pull/30078#issuecomment-2108094818)
I was able to make a depends build on macOS 14.4.1.

Guix hashes:

```
bdbb759d06e9766c5aa29a9ee1cea6f021d50618e1abe4732ae0120c4b444829 guix-build-019ad7327c39/output/aarch64-linux-gnu/SHA256SUMS.part
82aa7b4af365dde09b7fe435bd20432aa59168cb448f7a8cb898a8acb6178453 guix-build-019ad7327c39/output/aarch64-linux-gnu/bitcoin-019ad7327c39-aarch64-linux-gnu-debug.tar.gz
93c539e8c42ff767e46fc3f501cf9d543b954202372c930c177ff22eea6037f5 guix-build-019ad7327c39/output/aarch64-linux-gnu/bitcoin-0
...

💬 instagibbs commented on pull request "Cluster size 2 package rbf":
(https://github.com/bitcoin/bitcoin/pull/28984#discussion_r1598711206)
taken

💬 instagibbs commented on pull request "Cluster size 2 package rbf":
(https://github.com/bitcoin/bitcoin/pull/28984#discussion_r1598711279)
done

💬 instagibbs commented on pull request "Cluster size 2 package rbf":
(https://github.com/bitcoin/bitcoin/pull/28984#discussion_r1598711331)
picked a constant and used it

💬 instagibbs commented on pull request "Cluster size 2 package rbf":
(https://github.com/bitcoin/bitcoin/pull/28984#discussion_r1598711398)
removed a series of these ignored args

💬 instagibbs commented on pull request "Cluster size 2 package rbf":
(https://github.com/bitcoin/bitcoin/pull/28984#discussion_r1598711480)
Agreed, it's perhaps an older badly done version of #30066, removed

💬 maflcko commented on issue "RFC: "Insufficient review" tag for closed PRs":
(https://github.com/bitcoin/bitcoin/issues/29839#issuecomment-2108095272)
> Another idea would be to have a/the bot to automatically tag the PR with `insufficient review` if it gets stalled for a few weeks, before closing it, both to facilitate closing them for lack of activity but also to try to attract review effort for it.

Not sure if this can be done automatically, because evaluating whether review is sufficient or not may involve a broader understanding. For example, the existing review may have been sufficient, but a minor change was pushed and no one yet did
...