💬 alfonsoromanz commented on pull request "test: Assumeutxo: import snapshot in a node with a divergent chain":
(https://github.com/bitcoin/bitcoin/pull/29996#discussion_r1598651033)
Not sure. Maybe you can try opening an issue to discuss it

💬 alfonsoromanz commented on pull request "test: Assumeutxo: import snapshot in a node with a divergent chain":
(https://github.com/bitcoin/bitcoin/pull/29996#issuecomment-2107965861)
Rebased. I also addressed some feedback from rkrux's comments: changed hardcoded value of `nblocks` from 99 to a dynamic value, i.e `SNAPSHOT_BASE_HEIGHT-START_HEIGHT-1`

🤔 glozow reviewed a pull request: "26.x: backport #29853 ("sign: don't assume we are parsing a sane Miniscript")"
(https://github.com/bitcoin/bitcoin/pull/29854#pullrequestreview-2053051963)
Commit needs to be updated. I can add this to #29899 (to include the release notes changes as well) and close this?

⚠️ hebasto opened an issue: "windows: Newer libevent causes `http_request` fuzz target failure"
(https://github.com/bitcoin/bitcoin/issues/30096)
When building with MSVC, the `libevent` dependency package is provided by the vcpkg package manager.

The https://github.com/bitcoin/bitcoin/pull/27335 pinned the `libevent` version to [`2.1.12#7`](https://github.com/libevent/libevent/releases/tag/release-2.1.12-stable) to avoid issues with the changed signature of the `evhttp_connection_get_peer` function.

Then, https://github.com/bitcoin/bitcoin/pull/29774 introduced the `fuzz.exe` binary.

It turned out that the newer `libevent` versio
...

💬 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