Bitcoin Core Github
42 subscribers
124K links
Download Telegram
💬 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 commented on pull request "build: Introduce CMake-based build system":
(https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1742421067)
Fixed in https://github.com/bitcoin/bitcoin/pull/30803.
💬 hebasto commented on pull request "build: Introduce CMake-based build system":
(https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1742421343)
Fixed in https://github.com/bitcoin/bitcoin/pull/30803.
💬 hebasto commented on pull request "build: Introduce CMake-based build system":
(https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1742421605)
Fixed in https://github.com/bitcoin/bitcoin/pull/30803.
🤔 hebasto reviewed a pull request: "doc: Clarify libbitcoin_consensus in design/libraries.md"
(https://github.com/bitcoin/bitcoin/pull/30802#pullrequestreview-2278050191)
Concept ACK.
💬 hebasto commented on pull request "doc: Clarify libbitcoin_consensus in design/libraries.md":
(https://github.com/bitcoin/bitcoin/pull/30802#discussion_r1742425451)
Why `libbitcoin_node` removed? The diagram below still shows this dependency.
👍 maflcko approved a pull request: "build: Minor build system fixes and amendments"
(https://github.com/bitcoin/bitcoin/pull/30803#pullrequestreview-2278054511)
review ACK c393acadf213ddf58a69759eaf341595ccbf4889
💬 maflcko commented on pull request "build: Minor build system fixes and amendments":
(https://github.com/bitcoin/bitcoin/pull/30803#discussion_r1742427765)
```suggestion
# However, as of 2024, mainstream distributions do not yet provide
# CMake config files for ZeroMQ packages.
# If they do in the future, find_package(ZeroMQ) may be used instead.
```

nit: Could remove the TODO above and clarify that it is waiting on the distros.
💬 maflcko commented on pull request "doc: Clarify libbitcoin_consensus in design/libraries.md":
(https://github.com/bitcoin/bitcoin/pull/30802#discussion_r1742432173)
Well, it is used by everything that uses common. Probably best to just refer to the diagram.

Let me know if I should remove the "used by" completely.