💬 fjahr commented on pull request "rpc, rest: Improve block rpc error handling, check header before attempting to read block data.":
(https://github.com/bitcoin/bitcoin/pull/30410#discussion_r1742280583)
In 856c162fb6cdbb5715cae23d1b690d5bb3812afc: The function is introduced just two commits earlier, I think the `static` can be dropped there and the header entry added there as well.
(https://github.com/bitcoin/bitcoin/pull/30410#discussion_r1742280583)
In 856c162fb6cdbb5715cae23d1b690d5bb3812afc: The function is introduced just two commits earlier, I think the `static` can be dropped there and the header entry added there as well.
💬 hebasto commented on issue "cmake: incorrect assumption that `Debug` build type will not use optimisations":
(https://github.com/bitcoin/bitcoin/issues/30800#issuecomment-2326899886)
> However that isn't always true. If, for example, depends is built with `DEBUG=1`, and CMake is configured with `-DCMAKE_BUILD_TYPE=Debug`, `-O1` will actually be used.
I think we should first prioritize coordinating the optimization and debugging flags between the depends subsystem and the main build system:
1. Should the optimization and debugging flags coincide, or are they allowed to differ?
2. If the optimization and debugging can differ between the depends subsystem and in the main b
...
(https://github.com/bitcoin/bitcoin/issues/30800#issuecomment-2326899886)
> However that isn't always true. If, for example, depends is built with `DEBUG=1`, and CMake is configured with `-DCMAKE_BUILD_TYPE=Debug`, `-O1` will actually be used.
I think we should first prioritize coordinating the optimization and debugging flags between the depends subsystem and the main build system:
1. Should the optimization and debugging flags coincide, or are they allowed to differ?
2. If the optimization and debugging can differ between the depends subsystem and in the main b
...
💬 maflcko commented on pull request "build: Introduce CMake-based build system":
(https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1742328675)
`BUILD_FOR_FUZZING` has nothing to do with fuzz engines. According to the documentation it is an option to "Build for fuzzing. Enabling this will disable all other targets and override BUILD_FUZZ_BINARY."
Can you explain this a bit better? Maybe with an example output of a compile error or otherwise steps to reproduce?
(https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1742328675)
`BUILD_FOR_FUZZING` has nothing to do with fuzz engines. According to the documentation it is an option to "Build for fuzzing. Enabling this will disable all other targets and override BUILD_FUZZ_BINARY."
Can you explain this a bit better? Maybe with an example output of a compile error or otherwise steps to reproduce?
💬 hebasto commented on issue "cmake: Installed static kernel library is unusable":
(https://github.com/bitcoin/bitcoin/issues/30801#issuecomment-2326907165)
> I'd also wonder if it's really necessary to build a combined library, or better to just install the individual libraries we already have.
+1
(https://github.com/bitcoin/bitcoin/issues/30801#issuecomment-2326907165)
> I'd also wonder if it's really necessary to build a combined library, or better to just install the individual libraries we already have.
+1
💬 TheCharlatan commented on issue "cmake: Installed static kernel library is unusable":
(https://github.com/bitcoin/bitcoin/issues/30801#issuecomment-2326911621)
> Instead of target_link_libraries(bitcoinkernel bitcoin_crypto) do add_library(bitcoinkernel $<TARGET_OBJECTS:bitcoin_crypto>.
He, I think I was reading the same stack overflow post. I think this doesn't quite work, because those libraries have sub-dependencies that don't get included easily. So we'd need more logic to get the correct crc32 and crypto extensions if we attempt that patch. I'm not sure that would be really desirable, since we'd be duplicating a bunch of implementation selectio
...
(https://github.com/bitcoin/bitcoin/issues/30801#issuecomment-2326911621)
> Instead of target_link_libraries(bitcoinkernel bitcoin_crypto) do add_library(bitcoinkernel $<TARGET_OBJECTS:bitcoin_crypto>.
He, I think I was reading the same stack overflow post. I think this doesn't quite work, because those libraries have sub-dependencies that don't get included easily. So we'd need more logic to get the correct crc32 and crypto extensions if we attempt that patch. I'm not sure that would be really desirable, since we'd be duplicating a bunch of implementation selectio
...
💬 maflcko commented on pull request "build: Introduce CMake-based build system":
(https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1742339471)
Thanks for the reference. Looks like it was added in commit dc1e7ad7a5713d885f70ccc6c93e7a4c07e76559 by @ryanofsky. My preference would be to just drop the prefix "Stable, backwards-compatible", because it is unclear whether it refers to the build system artefacts, or the consensus rules. In either case, the prefix seems either wrong or an approximation that lacks nuance.
(https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1742339471)
Thanks for the reference. Looks like it was added in commit dc1e7ad7a5713d885f70ccc6c93e7a4c07e76559 by @ryanofsky. My preference would be to just drop the prefix "Stable, backwards-compatible", because it is unclear whether it refers to the build system artefacts, or the consensus rules. In either case, the prefix seems either wrong or an approximation that lacks nuance.
💬 l0rinc commented on pull request "validation: write chainstate to disk every hour":
(https://github.com/bitcoin/bitcoin/pull/30611#discussion_r1742340130)
what would happen when we restart with a different data directory, it will have to refetch everything, right? Or if we had a reorg on the very first block maybe?
Maybe @sipa can help us out here...
(https://github.com/bitcoin/bitcoin/pull/30611#discussion_r1742340130)
what would happen when we restart with a different data directory, it will have to refetch everything, right? Or if we had a reorg on the very first block maybe?
Maybe @sipa can help us out here...
💬 maflcko commented on pull request "build: Introduce CMake-based build system":
(https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1742342524)
Thanks. Makes sense. I guess the comment should be adjusted to say that this won't be possible to do any time soon, or at all, for the reason given by you?
(https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1742342524)
Thanks. Makes sense. I guess the comment should be adjusted to say that this won't be possible to do any time soon, or at all, for the reason given by you?
📝 tdb3 reopened a pull request: "rpc: add getorphantxs"
(https://github.com/bitcoin/bitcoin/pull/30793)
This PR adds a new rpc, `getorphantxs`, that provides the caller with a list of orphan transactions.
Currently looking to gauge interest on the concept, or any other productive feedback. If there is little/no interest, will close the PR for history.
PR https://github.com/bitcoin/bitcoin/pull/30784 adds a test that excessively large transactions aren't added to the orphanage. It checks the debug.log for confirmation that the transaction was not added.
As discussed in comment https://gi
...
(https://github.com/bitcoin/bitcoin/pull/30793)
This PR adds a new rpc, `getorphantxs`, that provides the caller with a list of orphan transactions.
Currently looking to gauge interest on the concept, or any other productive feedback. If there is little/no interest, will close the PR for history.
PR https://github.com/bitcoin/bitcoin/pull/30784 adds a test that excessively large transactions aren't added to the orphanage. It checks the debug.log for confirmation that the transaction was not added.
As discussed in comment https://gi
...
💬 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.
(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.
(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`).
(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
...
(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?
(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`?
(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?
(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 .
(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.
(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
...
(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?
(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?