💬 maflcko commented on pull request "build: Switch to Qt 6":
(https://github.com/bitcoin/bitcoin/pull/30997#issuecomment-2396830039)
IIUC macOS 11 does not receive security patches, so it may be better to remove it either way, see https://github.com/bitcoin/bitcoin/pull/30997#discussion_r1789897798
(https://github.com/bitcoin/bitcoin/pull/30997#issuecomment-2396830039)
IIUC macOS 11 does not receive security patches, so it may be better to remove it either way, see https://github.com/bitcoin/bitcoin/pull/30997#discussion_r1789897798
💬 Sjors commented on pull request "docs: Add instructions on how to self-sign bitcoin-core binaries for macOS":
(https://github.com/bitcoin/bitcoin/pull/30982#discussion_r1790160328)
This might be a bit confusing, because you don't need to self-sign the main drag & drop release.
(https://github.com/bitcoin/bitcoin/pull/30982#discussion_r1790160328)
This might be a bit confusing, because you don't need to self-sign the main drag & drop release.
💬 brunoerg commented on pull request "net: fuzz: bypass network magic and checksum validation":
(https://github.com/bitcoin/bitcoin/pull/31012#issuecomment-2396845482)
> Copy and pasting the command from the docs doesn't work:
Just fixed it. Can you check it?
(https://github.com/bitcoin/bitcoin/pull/31012#issuecomment-2396845482)
> Copy and pasting the command from the docs doesn't work:
Just fixed it. Can you check it?
💬 ismaelsadeeq commented on issue "[Testnet] Insufficient data or no feerate found":
(https://github.com/bitcoin/bitcoin/issues/31032#issuecomment-2396883367)
> I'd like to think that this is a problem caused by not having enough data, but that doesn't seem to be the case for me.
> I have two Bitcoin testnet nodes running (let's call them A and B).
> Sometimes node A throws the error "Insufficient data or no feerate found," while node B returns a feerate without any issue.
> Other times, both nodes A and B encounter the same error.
This behavior is inherent to the fee estimator, which collects data and decays it over time. As a result, you
...
(https://github.com/bitcoin/bitcoin/issues/31032#issuecomment-2396883367)
> I'd like to think that this is a problem caused by not having enough data, but that doesn't seem to be the case for me.
> I have two Bitcoin testnet nodes running (let's call them A and B).
> Sometimes node A throws the error "Insufficient data or no feerate found," while node B returns a feerate without any issue.
> Other times, both nodes A and B encounter the same error.
This behavior is inherent to the fee estimator, which collects data and decays it over time. As a result, you
...
💬 kevkevinpal commented on pull request "test: Assert that when we add the max orphan amount that we cannot add anymore and that a random orphan gets dropped":
(https://github.com/bitcoin/bitcoin/pull/31040#issuecomment-2396883806)
> Concept ACK Thanks, this is useful! I'm uncertain if this check is better suited for `p2p_orphan_handling` or `rpc_getorphantxs` (or for a `feature_orphanage` or `mempool_orphanage`). If `p2p_orphan_handling` is the preferred location, I'm happy to include your commit (preserving you as author) in #31037.
>
> https://github.com/bitcoin/bitcoin/blob/master/test/functional/README.md#naming-guidelines
>
> Can also check unit tests (https://github.com/bitcoin/bitcoin/blob/master/src/test/orp
...
(https://github.com/bitcoin/bitcoin/pull/31040#issuecomment-2396883806)
> Concept ACK Thanks, this is useful! I'm uncertain if this check is better suited for `p2p_orphan_handling` or `rpc_getorphantxs` (or for a `feature_orphanage` or `mempool_orphanage`). If `p2p_orphan_handling` is the preferred location, I'm happy to include your commit (preserving you as author) in #31037.
>
> https://github.com/bitcoin/bitcoin/blob/master/test/functional/README.md#naming-guidelines
>
> Can also check unit tests (https://github.com/bitcoin/bitcoin/blob/master/src/test/orp
...
💬 TheCharlatan commented on pull request "kernel: Move block tree db open to block manager":
(https://github.com/bitcoin/bitcoin/pull/30965#discussion_r1790201191)
See #31046
(https://github.com/bitcoin/bitcoin/pull/30965#discussion_r1790201191)
See #31046
📝 hebasto opened a pull request: "build: Bump minimum supported macOS to 12.0"
(https://github.com/bitcoin/bitcoin/pull/31048)
Running Bitcoin Core on unsupported OSes may expose users to security issues.
macOS Big Sur 11 received its last security update ([11.7.10](https://support.apple.com/en-us/106338)) over a year ago.
Addresses https://github.com/bitcoin/bitcoin/pull/30997#issuecomment-2396830039.
(https://github.com/bitcoin/bitcoin/pull/31048)
Running Bitcoin Core on unsupported OSes may expose users to security issues.
macOS Big Sur 11 received its last security update ([11.7.10](https://support.apple.com/en-us/106338)) over a year ago.
Addresses https://github.com/bitcoin/bitcoin/pull/30997#issuecomment-2396830039.
💬 vasild commented on issue "build: RFC Coverage build type":
(https://github.com/bitcoin/bitcoin/issues/31047#issuecomment-2396896391)
> our build system doesn't need to (and shouldn't have to) handle all possible combinations of compiler + tooling
It is just two:
1. gcc compiler + gcov/lcov and
2. clang compiler + llvm-cov (source based).
(I think it is a waste of time and we should ignore a third option where we try to use the gcov/lcov workflow with clang).
> Providing a single convenience build type that is the same as what is used by the majority of the developers seems fine
I do not object this, but then if
...
(https://github.com/bitcoin/bitcoin/issues/31047#issuecomment-2396896391)
> our build system doesn't need to (and shouldn't have to) handle all possible combinations of compiler + tooling
It is just two:
1. gcc compiler + gcov/lcov and
2. clang compiler + llvm-cov (source based).
(I think it is a waste of time and we should ignore a third option where we try to use the gcov/lcov workflow with clang).
> Providing a single convenience build type that is the same as what is used by the majority of the developers seems fine
I do not object this, but then if
...
💬 hebasto commented on pull request "build: Switch to Qt 6":
(https://github.com/bitcoin/bitcoin/pull/30997#issuecomment-2396896509)
> IIUC macOS 11 does not receive security patches, so it may be better to remove it either way, see [#30997 (comment)](https://github.com/bitcoin/bitcoin/pull/30997#discussion_r1789897798)
Addressed in https://github.com/bitcoin/bitcoin/pull/31048.
(https://github.com/bitcoin/bitcoin/pull/30997#issuecomment-2396896509)
> IIUC macOS 11 does not receive security patches, so it may be better to remove it either way, see [#30997 (comment)](https://github.com/bitcoin/bitcoin/pull/30997#discussion_r1789897798)
Addressed in https://github.com/bitcoin/bitcoin/pull/31048.
🤔 pablomartin4btc reviewed a pull request: "Decouple WalletModel from RPCExecutor"
(https://github.com/bitcoin-core/gui/pull/841#pullrequestreview-2351869077)
tACK 002b792b9a85100d89e47706c29cf1fd355d9727
Tested the rpc console with and without `-DENABLE_WALLET`. I think the refactor makes sense as the rpc just needs the wallet name for the related commands.
Out of the scope of this PR, I noticed that the order of the wallets in the combo-boxes corresponds to the `settings.json` file and if the user changes the wallet in the main window, when the rpc console is open, the wallet in the console combo doesn't correspond with the one in the main win
...
(https://github.com/bitcoin-core/gui/pull/841#pullrequestreview-2351869077)
tACK 002b792b9a85100d89e47706c29cf1fd355d9727
Tested the rpc console with and without `-DENABLE_WALLET`. I think the refactor makes sense as the rpc just needs the wallet name for the related commands.
Out of the scope of this PR, I noticed that the order of the wallets in the combo-boxes corresponds to the `settings.json` file and if the user changes the wallet in the main window, when the rpc console is open, the wallet in the console combo doesn't correspond with the one in the main win
...
💬 pablomartin4btc commented on pull request "Decouple WalletModel from RPCExecutor":
(https://github.com/bitcoin-core/gui/pull/841#discussion_r1790172791)
nit: describe wallet_name as a param[in] (wallet_model wasn't there either)
```suggestion
* @param[out] pstrFilteredOut Command line, filtered to remove any sensitive data
* @param[in] wallet_mame ...
```
(https://github.com/bitcoin-core/gui/pull/841#discussion_r1790172791)
nit: describe wallet_name as a param[in] (wallet_model wasn't there either)
```suggestion
* @param[out] pstrFilteredOut Command line, filtered to remove any sensitive data
* @param[in] wallet_mame ...
```
💬 fanquake commented on issue "build: RFC Coverage build type":
(https://github.com/bitcoin/bitcoin/issues/31047#issuecomment-2396911373)
> It is just two:
It's not though, because `gcc compiler + gcov/lcov` already encompasses more than 1 thing. i.e supporting different versions of gcov/lcov across releases.
(https://github.com/bitcoin/bitcoin/issues/31047#issuecomment-2396911373)
> It is just two:
It's not though, because `gcc compiler + gcov/lcov` already encompasses more than 1 thing. i.e supporting different versions of gcov/lcov across releases.
🤔 hebasto reviewed a pull request: "ci: set a ctest test timeout of 1200 (20 minutes)"
(https://github.com/bitcoin/bitcoin/pull/31026#pullrequestreview-2351928155)
Post-merge ACK 56aad83307e46983a397236bd0959e634207f83e.
(https://github.com/bitcoin/bitcoin/pull/31026#pullrequestreview-2351928155)
Post-merge ACK 56aad83307e46983a397236bd0959e634207f83e.
💬 andrewtoth commented on issue "Crash upon RPC v1 connection in v28.0.0":
(https://github.com/bitcoin/bitcoin/issues/31041#issuecomment-2396914856)
> Does it still happen after https://github.com/romanz/electrs/pull/1091 ?
That is for a different fork of electrs. For the mempool/electrs, you can see in the original logs in #31039 that it is already waiting for mempool to load before syncing.
(https://github.com/bitcoin/bitcoin/issues/31041#issuecomment-2396914856)
> Does it still happen after https://github.com/romanz/electrs/pull/1091 ?
That is for a different fork of electrs. For the mempool/electrs, you can see in the original logs in #31039 that it is already waiting for mempool to load before syncing.
💬 maflcko commented on pull request "Fix unsigned integer overflows in interpreter":
(https://github.com/bitcoin/bitcoin/pull/24214#issuecomment-2396926622)
> nit: might be better to use `static_cast` instead of the constructor-style cast.
Starting with C++11, `int64_t{something}` is actually preferred, because it documents (and checks at compile time) that no narrowing happens. `int64_t(something)` documents that the reviewer will have to check for possible narrowing.
(https://github.com/bitcoin/bitcoin/pull/24214#issuecomment-2396926622)
> nit: might be better to use `static_cast` instead of the constructor-style cast.
Starting with C++11, `int64_t{something}` is actually preferred, because it documents (and checks at compile time) that no narrowing happens. `int64_t(something)` documents that the reviewer will have to check for possible narrowing.
⚠️ fanquake opened an issue: "build: macOS fuzz instructions broken on 15.x"
(https://github.com/bitcoin/bitcoin/issues/31049)
Testing master at 62e4516722115c2d5aeb6c197abc73ca7c078b23 and the fuzzing.md instructions:
```bash
cmake --preset=libfuzzer \
-DCMAKE_C_COMPILER="$(brew --prefix llvm)/bin/clang" \
-DCMAKE_CXX_COMPILER="$(brew --prefix llvm)/bin/clang++" \
-DAPPEND_LDFLAGS=-Wl,-no_warn_duplicate_libraries
cmake --build build_fuzz
<snip>
[100%] Linking CXX executable fuzz
ld: multiple errors: invalid r_symbolnum=1 in '/Users/michael/fanquake-bitcoin/build_fuzz/src/test/fuzz/CMakeFiles/fuzz.d
...
(https://github.com/bitcoin/bitcoin/issues/31049)
Testing master at 62e4516722115c2d5aeb6c197abc73ca7c078b23 and the fuzzing.md instructions:
```bash
cmake --preset=libfuzzer \
-DCMAKE_C_COMPILER="$(brew --prefix llvm)/bin/clang" \
-DCMAKE_CXX_COMPILER="$(brew --prefix llvm)/bin/clang++" \
-DAPPEND_LDFLAGS=-Wl,-no_warn_duplicate_libraries
cmake --build build_fuzz
<snip>
[100%] Linking CXX executable fuzz
ld: multiple errors: invalid r_symbolnum=1 in '/Users/michael/fanquake-bitcoin/build_fuzz/src/test/fuzz/CMakeFiles/fuzz.d
...
💬 kevkevinpal commented on pull request "test: Assert that when we add the max orphan amount that we cannot add anymore and that a random orphan gets dropped":
(https://github.com/bitcoin/bitcoin/pull/31040#discussion_r1790246276)
Good catch! Just to make sure I ran it 100 times and on the 90th attempt it failed, I removed the assert in [33e4dc8](https://github.com/bitcoin/bitcoin/pull/31040/commits/33e4dc834f433a51d5a4a8ffa7ce81a98ac35b7a)
(https://github.com/bitcoin/bitcoin/pull/31040#discussion_r1790246276)
Good catch! Just to make sure I ran it 100 times and on the 90th attempt it failed, I removed the assert in [33e4dc8](https://github.com/bitcoin/bitcoin/pull/31040/commits/33e4dc834f433a51d5a4a8ffa7ce81a98ac35b7a)
💬 Sjors commented on pull request "build: Bump minimum supported macOS to 12.0":
(https://github.com/bitcoin/bitcoin/pull/31048#issuecomment-2396969984)
Concept ACK. See https://github.com/bitcoin/bitcoin/pull/30997#issuecomment-2382352367
(https://github.com/bitcoin/bitcoin/pull/31048#issuecomment-2396969984)
Concept ACK. See https://github.com/bitcoin/bitcoin/pull/30997#issuecomment-2382352367
💬 vasild commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#issuecomment-2396973132)
`54f4f3b05e...f9b2eaf96c`: rebase due to conflicts
(https://github.com/bitcoin/bitcoin/pull/29415#issuecomment-2396973132)
`54f4f3b05e...f9b2eaf96c`: rebase due to conflicts
💬 ismaelsadeeq commented on pull request "Fix unsigned integer overflows in interpreter":
(https://github.com/bitcoin/bitcoin/pull/24214#issuecomment-2397020325)
> Starting with C++11, `int64_t{something}` is actually preferred, because it documents (and checks at compile time) that no narrowing happens. `int64_t(something)` documents that the reviewer will have to check for possible narrowing.
Agreed I am talking about changing `size_t(int64_t(stack.size())` to `static_cast<size_t>(static_cast<int64_t>(stack.size())` same for `altstack` ?
(https://github.com/bitcoin/bitcoin/pull/24214#issuecomment-2397020325)
> Starting with C++11, `int64_t{something}` is actually preferred, because it documents (and checks at compile time) that no narrowing happens. `int64_t(something)` documents that the reviewer will have to check for possible narrowing.
Agreed I am talking about changing `size_t(int64_t(stack.size())` to `static_cast<size_t>(static_cast<int64_t>(stack.size())` same for `altstack` ?