💬 ismaelsadeeq commented on pull request "mining: bugfix: Fix duplicate coinbase tx weight reservation":
(https://github.com/bitcoin/bitcoin/pull/31384#discussion_r1864585362)
This is gone now, thanks.
(https://github.com/bitcoin/bitcoin/pull/31384#discussion_r1864585362)
This is gone now, thanks.
💬 ismaelsadeeq commented on pull request "mining: bugfix: Fix duplicate coinbase tx weight reservation":
(https://github.com/bitcoin/bitcoin/pull/31384#issuecomment-2509452312)
Thanks for your review, @Sjors.
I addressed all of your comments.
> Something like this should do
Done. Instead of creating `DEFAULT_BLOCK_MAX_WEIGHT` that copies `MAX_BLOCK_WEIGHT`, I've used it directly.
> Maybe init.cpp should also enforce that -blockmaxweight <= MAX_BLOCK_WEIGHT
We already clamp the block size in the block assembler, so it will never exceed `MAX_BLOCK_WEIGHT`, but done with a test.
I've added a functional test and release notes.
(https://github.com/bitcoin/bitcoin/pull/31384#issuecomment-2509452312)
Thanks for your review, @Sjors.
I addressed all of your comments.
> Something like this should do
Done. Instead of creating `DEFAULT_BLOCK_MAX_WEIGHT` that copies `MAX_BLOCK_WEIGHT`, I've used it directly.
> Maybe init.cpp should also enforce that -blockmaxweight <= MAX_BLOCK_WEIGHT
We already clamp the block size in the block assembler, so it will never exceed `MAX_BLOCK_WEIGHT`, but done with a test.
I've added a functional test and release notes.
📝 theStack opened a pull request: "wallet: refactor: various master key encryption cleanups"
(https://github.com/bitcoin/bitcoin/pull/31398)
This PR contains various cleanups around the wallet's master key encryption logic. The default/minimum key derivation rounds magic number of 25000 is hoisted into a constant (member of `CMasterKey`) and two new functions `EncryptMasterKey`/`DecryptMasterKey` are introduced in order to deduplicate code for the derivation round determination and master key en/decryption. Also, mentions of the never-implemented derivation method `scrypt` are removed from the wallet crypter header and both plain and
...
(https://github.com/bitcoin/bitcoin/pull/31398)
This PR contains various cleanups around the wallet's master key encryption logic. The default/minimum key derivation rounds magic number of 25000 is hoisted into a constant (member of `CMasterKey`) and two new functions `EncryptMasterKey`/`DecryptMasterKey` are introduced in order to deduplicate code for the derivation round determination and master key en/decryption. Also, mentions of the never-implemented derivation method `scrypt` are removed from the wallet crypter header and both plain and
...
💬 davidgumberg commented on pull request "dbwrapper: Bump LevelDB max file size to 32 MiB to avoid system slowdown from high disk cache flush rate":
(https://github.com/bitcoin/bitcoin/pull/30039#issuecomment-2509581003)
ACK https://github.com/bitcoin/bitcoin/commit/b73d3319377a4c9d7e2dd279c3d106002585bc36
(https://github.com/bitcoin/bitcoin/pull/30039#issuecomment-2509581003)
ACK https://github.com/bitcoin/bitcoin/commit/b73d3319377a4c9d7e2dd279c3d106002585bc36
💬 purpleKarrot commented on pull request "cmake: add optional source files to bitcoin_crypto directly":
(https://github.com/bitcoin/bitcoin/pull/31268#issuecomment-2509610176)
> If source properties are used to modify the compiler flags for specific source files rather than for a whole target, changing the source’s compiler flags will still result in all of the target’s sources being rebuilt, not just the affected source file.
I don't think that applies here. The source's compile flags are never changed between builds: `sha256_avx2.cpp` is either compiled with `${AVX2_CXXFLAGS}`, or it is not compiled at all. And `AVX2_CXXFLAGS` is only ever set to `-mavx -mavx2`.
...
(https://github.com/bitcoin/bitcoin/pull/31268#issuecomment-2509610176)
> If source properties are used to modify the compiler flags for specific source files rather than for a whole target, changing the source’s compiler flags will still result in all of the target’s sources being rebuilt, not just the affected source file.
I don't think that applies here. The source's compile flags are never changed between builds: `sha256_avx2.cpp` is either compiled with `${AVX2_CXXFLAGS}`, or it is not compiled at all. And `AVX2_CXXFLAGS` is only ever set to `-mavx -mavx2`.
...
👍 hebasto approved a pull request: "build: Set shared linker flags in toolchain file"
(https://github.com/bitcoin/bitcoin/pull/31395#pullrequestreview-2471346690)
ACK a8e04704f930de168fdddbb1c040dd70fbbe8ff7, tested on Ubuntu 24.04:
```
$ make -C depends -j 16 HOST=arm64-apple-darwin NO_QT=1
$ cmake -B build --toolchain depends/arm64-apple-darwin/toolchain.cmake -DBUILD_SHARED_LIBS=ON -DBUILD_KERNEL_LIB=ON
$ cmake --build build -j 16 -t bitcoinkernel
```
(https://github.com/bitcoin/bitcoin/pull/31395#pullrequestreview-2471346690)
ACK a8e04704f930de168fdddbb1c040dd70fbbe8ff7, tested on Ubuntu 24.04:
```
$ make -C depends -j 16 HOST=arm64-apple-darwin NO_QT=1
$ cmake -B build --toolchain depends/arm64-apple-darwin/toolchain.cmake -DBUILD_SHARED_LIBS=ON -DBUILD_KERNEL_LIB=ON
$ cmake --build build -j 16 -t bitcoinkernel
```
💬 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-2509662129)
Rebased https://github.com/bitcoin/bitcoin/commit/19f39271fa9f5b26075ca8f66afe25751813bb75 to https://github.com/bitcoin/bitcoin/commit/ef7b1e8e82819e5104c1f1f49f46ea74b416ced0
>This PR implements a fix for the issue described in https://github.com/bitcoin/bitcoin/issues/29435.
>
>The problem is that the wallet is unable to abandon transactions that have unrelated parent conflicts. The solution implemented here augments the mempool transaction REPLACED signal with the double-spending transa
...
(https://github.com/bitcoin/bitcoin/pull/29680#issuecomment-2509662129)
Rebased https://github.com/bitcoin/bitcoin/commit/19f39271fa9f5b26075ca8f66afe25751813bb75 to https://github.com/bitcoin/bitcoin/commit/ef7b1e8e82819e5104c1f1f49f46ea74b416ced0
>This PR implements a fix for the issue described in https://github.com/bitcoin/bitcoin/issues/29435.
>
>The problem is that the wallet is unable to abandon transactions that have unrelated parent conflicts. The solution implemented here augments the mempool transaction REPLACED signal with the double-spending transa
...
💬 hebasto commented on pull request "cmake: add optional source files to bitcoin_crypto directly":
(https://github.com/bitcoin/bitcoin/pull/31268#issuecomment-2509680979)
> @hebasto, can you elaborate why you think the source-specific compile options are hard to reason about?
Sure. Consider this minimal example:
```
cmake_minimum_required(VERSION 3.22)
project(test CXX)
add_library(interface INTERFACE)
target_compile_options(interface INTERFACE -Wno-mock-interface)
add_executable(test main.cpp)
set_property(SOURCE main.cpp PROPERTY COMPILE_OPTIONS -Wno-mock-source)
target_compile_options(test PRIVATE -Wno-mock-target)
target_link_libraries(test PR
...
(https://github.com/bitcoin/bitcoin/pull/31268#issuecomment-2509680979)
> @hebasto, can you elaborate why you think the source-specific compile options are hard to reason about?
Sure. Consider this minimal example:
```
cmake_minimum_required(VERSION 3.22)
project(test CXX)
add_library(interface INTERFACE)
target_compile_options(interface INTERFACE -Wno-mock-interface)
add_executable(test main.cpp)
set_property(SOURCE main.cpp PROPERTY COMPILE_OPTIONS -Wno-mock-source)
target_compile_options(test PRIVATE -Wno-mock-target)
target_link_libraries(test PR
...
📝 hebasto opened a pull request: "ci, macos: Install `pkgconf` Homebrew's package"
(https://github.com/bitcoin/bitcoin/pull/31399)
The updated GHA image [`20241125.556`](https://github.com/actions/runner-images/releases/tag/macos-14-arm64%2F20241125.556) is now fully [deployed](https://github.com/actions/runner-images/blob/main/README.md#available-images), enabling the installation of `pkgconf` as documented in [`doc/build-osx.md`](https://github.com/bitcoin/bitcoin/blob/master/doc/build-osx.md#3-install-required-dependencies).
(https://github.com/bitcoin/bitcoin/pull/31399)
The updated GHA image [`20241125.556`](https://github.com/actions/runner-images/releases/tag/macos-14-arm64%2F20241125.556) is now fully [deployed](https://github.com/actions/runner-images/blob/main/README.md#available-images), enabling the installation of `pkgconf` as documented in [`doc/build-osx.md`](https://github.com/bitcoin/bitcoin/blob/master/doc/build-osx.md#3-install-required-dependencies).
💬 purpleKarrot commented on pull request "cmake: add optional source files to bitcoin_crypto directly":
(https://github.com/bitcoin/bitcoin/pull/31268#issuecomment-2509724461)
Why is that hard to reason about? If I add
```cmake
add_compile_options(-Wno-mock-global)
```
the resulting order will be `-Wno-mock-global -Wno-mock-target -Wno-mock-interface -Wno-mock-source`, so the flags are ordered from most generic to most specific.
That aside, I don't think the order of arguments is relevant here.
(https://github.com/bitcoin/bitcoin/pull/31268#issuecomment-2509724461)
Why is that hard to reason about? If I add
```cmake
add_compile_options(-Wno-mock-global)
```
the resulting order will be `-Wno-mock-global -Wno-mock-target -Wno-mock-interface -Wno-mock-source`, so the flags are ordered from most generic to most specific.
That aside, I don't think the order of arguments is relevant here.
💬 hebasto commented on pull request "cmake: add optional source files to bitcoin_crypto directly":
(https://github.com/bitcoin/bitcoin/pull/31268#issuecomment-2509726671)
> Why is that hard to reason about?
People unfamiliar with such peculiarities might not realise that compiler options specified as a target's property take precedence over other compiler options.
(https://github.com/bitcoin/bitcoin/pull/31268#issuecomment-2509726671)
> Why is that hard to reason about?
People unfamiliar with such peculiarities might not realise that compiler options specified as a target's property take precedence over other compiler options.
💬 hebasto commented on issue "build: Windows debug cross-build fails with `-O0`":
(https://github.com/bitcoin/bitcoin/issues/28109#issuecomment-2509735314)
Now observing a similar error when building on Windows with the default `--config Debug`:
```
...\bitcoin\src\wallet\wallet.cpp(1,1): error C1128: number of sections exceeded object file format limit: compile with /bigobj [...\bitcoin\build-static\src\wallet\bitcoin_wallet.vcxproj]
```
(https://github.com/bitcoin/bitcoin/issues/28109#issuecomment-2509735314)
Now observing a similar error when building on Windows with the default `--config Debug`:
```
...\bitcoin\src\wallet\wallet.cpp(1,1): error C1128: number of sections exceeded object file format limit: compile with /bigobj [...\bitcoin\build-static\src\wallet\bitcoin_wallet.vcxproj]
```
💬 laanwj commented on pull request "Add and use `satToBtc` and `btcToSat` util functions":
(https://github.com/bitcoin/bitcoin/pull/31356#discussion_r1864872630)
An alternative idea would be to add a `btcStringToSat` function, instead of overloading `btcToSat` on type, as this makes type checking harder.
(https://github.com/bitcoin/bitcoin/pull/31356#discussion_r1864872630)
An alternative idea would be to add a `btcStringToSat` function, instead of overloading `btcToSat` on type, as this makes type checking harder.
💬 laanwj commented on pull request "Add and use `satToBtc` and `btcToSat` util functions":
(https://github.com/bitcoin/bitcoin/pull/31356#discussion_r1864872826)
Yes, passing sats as a decimal should never be necessary.
(https://github.com/bitcoin/bitcoin/pull/31356#discussion_r1864872826)
Yes, passing sats as a decimal should never be necessary.
💬 purpleKarrot commented on pull request "cmake: add optional source files to bitcoin_crypto directly":
(https://github.com/bitcoin/bitcoin/pull/31268#issuecomment-2509746149)
Yeah, they might also not realize that compiler options specified at the target level might take precedence over those specified globally...
(https://github.com/bitcoin/bitcoin/pull/31268#issuecomment-2509746149)
Yeah, they might also not realize that compiler options specified at the target level might take precedence over those specified globally...
📝 suriyaa opened a pull request: "doc: Update license year range to 2025"
(https://github.com/bitcoin/bitcoin/pull/31400)
<!--
*** Please remove the following help text before submitting: ***
Pull requests without a rationale and clear improvement may be closed
immediately.
GUI-related pull requests should be opened against
https://github.com/bitcoin-core/gui
first. See CONTRIBUTING.md
-->
**Just a trivial change by updating the year.**
*Comparable to older PRs such as https://github.com/bitcoin/bitcoin/pull/29222.*
(This PR can be merged by a Bitcoin Core maintainer at the end of year 2024 or at
...
(https://github.com/bitcoin/bitcoin/pull/31400)
<!--
*** Please remove the following help text before submitting: ***
Pull requests without a rationale and clear improvement may be closed
immediately.
GUI-related pull requests should be opened against
https://github.com/bitcoin-core/gui
first. See CONTRIBUTING.md
-->
**Just a trivial change by updating the year.**
*Comparable to older PRs such as https://github.com/bitcoin/bitcoin/pull/29222.*
(This PR can be merged by a Bitcoin Core maintainer at the end of year 2024 or at
...
💬 l0rinc commented on pull request "doc: Update license year range to 2025":
(https://github.com/bitcoin/bitcoin/pull/31400#issuecomment-2509755566)
Nack - we don't usually update copyright headers one-by-one.
(https://github.com/bitcoin/bitcoin/pull/31400#issuecomment-2509755566)
Nack - we don't usually update copyright headers one-by-one.
💬 hebasto commented on pull request "util: Drop boost posix_time in ParseISO8601DateTime":
(https://github.com/bitcoin/bitcoin/pull/31391#issuecomment-2509829044)
> Looks like MSVC is broken, according to the GHA run:
>
> ```
> D:/a/bitcoin/bitcoin/src/test/util_tests.cpp(334): error: in "util_tests/util_ParseISO8601DateTime": check ParseISO8601DateTime("9999-12-31T23:59:59Z").value() == 253402300799 has failed [-4295736961 != 253402300799]
> ```
>
> It would be good if a someone using Windows reproduced this locally and then reported the bug to MSVC. Godbolt draft: https://godbolt.org/z/4WYn6E61W
See https://developercommunity.visualstudio.com
...
(https://github.com/bitcoin/bitcoin/pull/31391#issuecomment-2509829044)
> Looks like MSVC is broken, according to the GHA run:
>
> ```
> D:/a/bitcoin/bitcoin/src/test/util_tests.cpp(334): error: in "util_tests/util_ParseISO8601DateTime": check ParseISO8601DateTime("9999-12-31T23:59:59Z").value() == 253402300799 has failed [-4295736961 != 253402300799]
> ```
>
> It would be good if a someone using Windows reproduced this locally and then reported the bug to MSVC. Godbolt draft: https://godbolt.org/z/4WYn6E61W
See https://developercommunity.visualstudio.com
...
👍 TheCharlatan approved a pull request: "ci, macos: Install `pkgconf` Homebrew's package"
(https://github.com/bitcoin/bitcoin/pull/31399#pullrequestreview-2471453504)
Lgtm ACK e2f2698395c2535eeada8d91f270643c77aa7099
(https://github.com/bitcoin/bitcoin/pull/31399#pullrequestreview-2471453504)
Lgtm ACK e2f2698395c2535eeada8d91f270643c77aa7099
💬 glozow commented on pull request "Package Relay 1/3: Introduce TxDownloadManager and improve orphan-handling":
(https://github.com/bitcoin/bitcoin/pull/28031#issuecomment-2510096748)
Rebased. I have split the first 12 commits (the majority of this PR) into #31397. The other 5 commits are for making the module internally thread-safe, and can be opened in another PR afterward.
(https://github.com/bitcoin/bitcoin/pull/28031#issuecomment-2510096748)
Rebased. I have split the first 12 commits (the majority of this PR) into #31397. The other 5 commits are for making the module internally thread-safe, and can be opened in another PR afterward.