💬 maflcko commented on pull request "build: Introduce CMake-based build system":
(https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1742348150)
Thanks for testing @fanquake. Just to clarify: I left the review comment, because running the tests was added as part of this pull request, even though it wasn't present previously with autotools.

💬 tdb3 commented on pull request "rpc: add getorphantxs":
(https://github.com/bitcoin/bitcoin/pull/30793#discussion_r1742352471)
Right. Initially, the thought was to break clean from PeerManager/TxOrphanage, but CTransactionRef is a shared_ptr so could access the CTransaction even if it was dropped in TxOrphanage.

🤔 mzumsande reviewed a pull request: "validation: do not wipe utxo cache for stats/scans/snapshots"
(https://github.com/bitcoin/bitcoin/pull/30610#pullrequestreview-2277942711)
Concept ACK

The test `interface_usdt_utxocache.py` fails because `generate()` in `wallet.py` calls `scantxoutset`, which doesn't wipe the cache anymore. As a result, the number of utxos flushed to disk in later flushes changes.

Re: "currently-unused CreateUTXOSnapshot" in the PR description - it's used to create snapshots (`dumptxoutset`).

💬 hebasto commented on pull request "build: Fix linking for `fuzz` target when building with MSan":
(https://github.com/bitcoin/bitcoin/pull/30778#discussion_r1742355241)
Sure. The logic is follows:
1. The `MSAN_FLAGS` variable's value is `... -g -O1 ...`.
2. This value is assigned to `CXXFLAGS` for building depends and become the value of `CMAKE_CXX_FLAGS` in the toolchain file.
3. Being configured with `-DCMAKE_BUILD_TYPE=Debug`, the build system may assign `-O0 -ftrapv -g3` to `CMAKE_CXX_FLAGS_DEBUG`.
4. When invoking a compier, `CMAKE_CXX_FLAGS` and `CMAKE_CXX_FLAGS_DEBUG` are concatenated, resulting in `.. -g -O1 ... -O0 ... -g3 ...`. This means that fla
...

💬 tdb3 commented on pull request "rpc: add getorphantxs":
(https://github.com/bitcoin/bitcoin/pull/30793#discussion_r1742358615)
Good suggestion. Updated to an array of objects, with each object containing txid and wtixd. Some fields that would think make sense to add:

- Expiration time
- Size info
- Fee info
- Originator (what sort of info would we like to see here? address?)

Alternatively, maybe could replace the wrapping array with an object, with each orphan being keyed by its wtixd?

💬 maflcko commented on pull request "rpc: add getorphantxs":
(https://github.com/bitcoin/bitcoin/pull/30793#discussion_r1742358892)
Maybe this can be marked Experimental, so that the interface can easily be changed, given that the goal would mostly only be for testing/debugging? Also, the RPC could be hidden, like other similar RPC, for example `estimaterawfee`?

💬 tdb3 commented on pull request "rpc: add getorphantxs":
(https://github.com/bitcoin/bitcoin/pull/30793#discussion_r1742363742)
Thank you. Both of those suggestions are reasonable to me. Will update soon. Do you have a preference?

💬 fjahr commented on pull request "assumeutxo: Add dumptxoutset height param, remove shell scripts":
(https://github.com/bitcoin/bitcoin/pull/29553#issuecomment-2326985542)
There are now 3 ACKs so unless need to retouch I will make a quick follow-up addressing the open comments from @pablomartin4btc and @mzumsande .

💬 maflcko commented on pull request "Remove unsafe uint256S() and test-only uint160S()":
(https://github.com/bitcoin/bitcoin/pull/30773#discussion_r1742381310)
I am mostly thinking about a user running into a copy-paste error, where they delete or add a character from a 64-length string. Returning them back the string won't be useful for them to spot the error, as they likely can already see their input. However telling them that they accidentally added or removed a character may be useful and saves them from manually counting the characters, or write a program to count for them.

💬 ryanofsky commented on issue "cmake: Installed static kernel library is unusable":
(https://github.com/bitcoin/bitcoin/issues/30801#issuecomment-2326995962)
I see. It does seem like cmake doesn't want to support "convenience libraries" (static libraries which are combinations of other static libraries) in the same way that libtool does. Which is kind of understandable because it would complicate dependency tracking and result in multiple libraries with duplicate copies of the same symbols which waste space and might not be safely linkable together.

Maybe using TARGET_OBJECTS and manually specifying recursive library dependencies would be a workab
...

💬 hebasto commented on pull request "build: Introduce CMake-based build system":
(https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1742389705)
This is used for the `check-symbols` and `check-security` targets, which are intended to be run in a Guix environment during the release process. Currently, we do not support the multiprocess feature in this workflow, right?

🤔 pablomartin4btc reviewed a pull request: "rpc: add getdescriptoractivity"
(https://github.com/bitcoin/bitcoin/pull/30708#pullrequestreview-2278000353)
Concept ACK

Performed some light testing with the given examples from the top description.

For more complex cases/ non standard descriptors, as mentioned by @furszy, perhaps it's worth it to analyze them later at some point.

🤔 pablomartin4btc reviewed a pull request: "rpc: add `revelant_blocks` to `scanblocks status`"
(https://github.com/bitcoin/bitcoin/pull/30713#pullrequestreview-2278003474)
Concept ACK (coming from #30708).

💬 ryanofsky commented on pull request "build: Fix linking for `fuzz` target when building with MSan":
(https://github.com/bitcoin/bitcoin/pull/30778#discussion_r1742403459)
Very helpful, thanks. I think I would write comment as "Set CMAKE_{C,CXX}_FLAGS_DEBUG options to empty so CMake's default options for debug builds do not interfere with MSAN flags"

📝 maflcko opened a pull request: "doc: Clarify libbitcoin_consensus in design/libraries.md"
(https://github.com/bitcoin/bitcoin/pull/30802)
Now that the shared library has been removed in commit 80f8b92f4f2311b9e9a25361c9dd973244e6f95c, update the documentation to drop the no-longer applicable prefix "Stable...".

💬 Sjors commented on issue "Testnet4 consensus failure due to timewarp related "softfork"":
(https://github.com/bitcoin/bitcoin/issues/30786#issuecomment-2327029294)
It's too hot to turn on my S9 :-)

💬 maflcko commented on pull request "build: Introduce CMake-based build system":
(https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1742404263)
Fixed in https://github.com/bitcoin/bitcoin/pull/30802

💬 maflcko commented on pull request "build: Introduce CMake-based build system":
(https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1742404977)
Fixed in https://github.com/bitcoin/bitcoin/pull/30802

👍 TheCharlatan approved a pull request: "doc: Clarify libbitcoin_consensus in design/libraries.md"
(https://github.com/bitcoin/bitcoin/pull/30802#pullrequestreview-2278039267)
ACK fad22fcc83bc009a8c3892cac3570cd399ae28f7

📝 hebasto opened a pull request: "build: Minor build syystem fixes and amendments"
(https://github.com/bitcoin/bitcoin/pull/30803)
This PR addresses the following comments:
- https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1742342524
- https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1728692369
- https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1736110362