📝 vasild opened a pull request: "util: avoid using thread_local variable that has a destructor"
(https://github.com/bitcoin/bitcoin/pull/30095)
Store the thread name in a `thread_local` variable of type `char[]` instead of `std::string`. This avoids calling the destructor when the thread exits. This is a workaround for
https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=278701

For type-safety, return `std::string_view` from
`util::ThreadGetInternalName()` instead of `char[]`.

As a side effect of this change, we no longer store a reference to a `thread_local` variable in `CLockLocation`. This was dangerous because if the thread qui
...

💬 vasild commented on issue "Rethink thread_local (take 2)":
(https://github.com/bitcoin/bitcoin/issues/29952#issuecomment-2107921564)
> Is anybody interested in reviewing the patch I posted above if I PR it: [#29952 (comment)](https://github.com/bitcoin/bitcoin/issues/29952#issuecomment-2079776241)

PRed at https://github.com/bitcoin/bitcoin/pull/30095

💬 vasild commented on pull request "util: avoid using thread_local variable that has a destructor":
(https://github.com/bitcoin/bitcoin/pull/30095#issuecomment-2107925627)
This came from the discussion in https://github.com/bitcoin/bitcoin/issues/29952

💬 Eunovo 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-2107935583)
> I don't think so: only the Conflict and Replace reasons need extra data, and from what I understand it can only be one reason at any given time. Given that, you could have a field on the new class for removal data which is a std::variant of the possible data types. For example, `std::variant<CTransactionRef, BlockData>`, where block data holds the block hash and block number.

@josibake Makes sense but I'm skeptical about how this affects the removal reason usage. For example, you can just d
...

💬 brunoerg commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r1598646404)
In 0ba30dec085f860c89f4fd8c8a398aa8a13b5ebd "net: implement opening PRIVATE_BROADCAST connections": `m_max_private_broadcast` is not used.

💬 alfonsoromanz commented on pull request "test: Assumeutxo: import snapshot in a node with a divergent chain":
(https://github.com/bitcoin/bitcoin/pull/29996#discussion_r1598649148)
Done. Thanks

💬 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