💬 itornaza commented on pull request "depends: fix for llvm-ranlib (etc): 'No such file or directory' macOS 15.0":
(https://github.com/bitcoin/bitcoin/pull/30994#issuecomment-2394069849)
> Not sure about this issue, or making this change. Looks like it needs more investigation. See my comment here: [#30978 (comment)](https://github.com/bitcoin/bitcoin/issues/30978#issuecomment-2388086723).
Yes, switched to draft until we have a better understanding of the issue.
(https://github.com/bitcoin/bitcoin/pull/30994#issuecomment-2394069849)
> Not sure about this issue, or making this change. Looks like it needs more investigation. See my comment here: [#30978 (comment)](https://github.com/bitcoin/bitcoin/issues/30978#issuecomment-2388086723).
Yes, switched to draft until we have a better understanding of the issue.
💬 theuni commented on pull request "fuzz: Add fuzz-only build mode option for targets":
(https://github.com/bitcoin/bitcoin/pull/31028#issuecomment-2394111449)
If we already know at compile-time that `FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION` is required, is there any value in registering `p2p_headers_presync` as a target if it's not set? Maybe a new `FUZZ_BUILD_ONLY_TARGET` or so, which would conditionally register?
(https://github.com/bitcoin/bitcoin/pull/31028#issuecomment-2394111449)
If we already know at compile-time that `FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION` is required, is there any value in registering `p2p_headers_presync` as a target if it's not set? Maybe a new `FUZZ_BUILD_ONLY_TARGET` or so, which would conditionally register?
👍 theuni approved a pull request: "ci: set a ctest test timeout of 1200 (20 minutes)"
(https://github.com/bitcoin/bitcoin/pull/31026#pullrequestreview-2348619977)
ACK 93dda4c70254750a5b5c0e7f7d2d89032519b999
(https://github.com/bitcoin/bitcoin/pull/31026#pullrequestreview-2348619977)
ACK 93dda4c70254750a5b5c0e7f7d2d89032519b999
💬 Sjors commented on pull request "multiprocess: add bitcoin-mine test program":
(https://github.com/bitcoin/bitcoin/pull/30437#issuecomment-2394194530)
In order to get these in the guix build, I think you need to move `set(installable_targets)` up in the file, and also add `bitcoin-mine` to the list in `foreach(target IN ITEMS bitcoind` in `Maintenance.cmake`. Though I'm not sure why that doesn't use `installable_targets`, cc @hebasto.
Maybe add a placeholder man page for `gen-manpages.py`.
(https://github.com/bitcoin/bitcoin/pull/30437#issuecomment-2394194530)
In order to get these in the guix build, I think you need to move `set(installable_targets)` up in the file, and also add `bitcoin-mine` to the list in `foreach(target IN ITEMS bitcoind` in `Maintenance.cmake`. Though I'm not sure why that doesn't use `installable_targets`, cc @hebasto.
Maybe add a placeholder man page for `gen-manpages.py`.
💬 Sjors commented on pull request "multiprocess: add bitcoin-mine test program":
(https://github.com/bitcoin/bitcoin/pull/30437#discussion_r1788103315)
Can you move this above `AddArgs`? In https://github.com/Sjors/bitcoin/pull/48 I'm adding some argument documentation that need accsess to `BaseParams`.
(https://github.com/bitcoin/bitcoin/pull/30437#discussion_r1788103315)
Can you move this above `AddArgs`? In https://github.com/Sjors/bitcoin/pull/48 I'm adding some argument documentation that need accsess to `BaseParams`.
💬 Sjors commented on pull request "Add multiprocess binaries to release build":
(https://github.com/bitcoin/bitcoin/pull/30975#discussion_r1788116010)
03e8fe41c3500c29cd003170f7346872227f3ca5: now that this breaking change is merged in libmultiprocess it would be good to have a PR to apply this on master.
(https://github.com/bitcoin/bitcoin/pull/30975#discussion_r1788116010)
03e8fe41c3500c29cd003170f7346872227f3ca5: now that this breaking change is merged in libmultiprocess it would be good to have a PR to apply this on master.
💬 mzumsande commented on issue "ci: `feature_settings.py` failed with `Invalid value detected for '-wallet' or '-nowallet'` in macOS 14 CI":
(https://github.com/bitcoin/bitcoin/issues/31019#issuecomment-2394368410)
I think the problem is that the node got stuck during shutdown for some reason:
```
node0 2024-10-02T00:15:45.784129Z [http] [src/httpserver.cpp:357] [ThreadHTTP] [http] Exited http event loop
test 2024-10-02T00:55:45.674000Z TestFramework (ERROR): Assertion failed
```
The second line is ~40 minutes after the first, the node hasn't completed shutdown during that time.
The node is expected to fail with `Invalid value detected for '-wallet' or '-nowallet'` (that's what the test is tes
...
(https://github.com/bitcoin/bitcoin/issues/31019#issuecomment-2394368410)
I think the problem is that the node got stuck during shutdown for some reason:
```
node0 2024-10-02T00:15:45.784129Z [http] [src/httpserver.cpp:357] [ThreadHTTP] [http] Exited http event loop
test 2024-10-02T00:55:45.674000Z TestFramework (ERROR): Assertion failed
```
The second line is ~40 minutes after the first, the node hasn't completed shutdown during that time.
The node is expected to fail with `Invalid value detected for '-wallet' or '-nowallet'` (that's what the test is tes
...
💬 Christewart commented on pull request "feefrac: add support for evaluating at given size":
(https://github.com/bitcoin/bitcoin/pull/30535#issuecomment-2394374445)
concept ACK
(https://github.com/bitcoin/bitcoin/pull/30535#issuecomment-2394374445)
concept ACK
💬 tdb3 commented on issue "ci: `feature_settings.py` failed in macOS 14 CI":
(https://github.com/bitcoin/bitcoin/issues/31019#issuecomment-2394376294)
> The node is expected to fail with `Invalid value detected for '-wallet' or '-nowallet'` (that's what the test is testing), so that's not the problem.
Right, the title was poorly worded. Updated
(https://github.com/bitcoin/bitcoin/issues/31019#issuecomment-2394376294)
> The node is expected to fail with `Invalid value detected for '-wallet' or '-nowallet'` (that's what the test is testing), so that's not the problem.
Right, the title was poorly worded. Updated
💬 davidgumberg commented on pull request "dbwrapper: Bump LevelDB max file size to 128 MiB to avoid system slowdown from high disk cache flush rate":
(https://github.com/bitcoin/bitcoin/pull/30039#issuecomment-2394400423)
I cherry picked your branch onto master and did two runs syncing from a stable, dedicated local node twice on a Raspberry Pi 5 4GB using microSD for storage, with a prune of `2000` and the default dbcache using the following command:
```
./build/src/bitcoind -daemon=0 -connect=ryzen7900xnode:8333 -stopatheight=800000 -prune=2000 -debug=bench -debug=blockstorage -debug=coindb -debug=mempool -debug=prune
```
I saw a *massive* improvement, with your branch taking, on average, **~6
...
(https://github.com/bitcoin/bitcoin/pull/30039#issuecomment-2394400423)
I cherry picked your branch onto master and did two runs syncing from a stable, dedicated local node twice on a Raspberry Pi 5 4GB using microSD for storage, with a prune of `2000` and the default dbcache using the following command:
```
./build/src/bitcoind -daemon=0 -connect=ryzen7900xnode:8333 -stopatheight=800000 -prune=2000 -debug=bench -debug=blockstorage -debug=coindb -debug=mempool -debug=prune
```
I saw a *massive* improvement, with your branch taking, on average, **~6
...
💬 davidgumberg commented on pull request "rpc: add getorphantxs":
(https://github.com/bitcoin/bitcoin/pull/30793#discussion_r1788273274)
non-blocking out-of-scope: Might be worth it to either deprecate true/false values for verbosity arguments or introduce an argument type specific to verbosity arguments to avoid skipping type checks for them.
(https://github.com/bitcoin/bitcoin/pull/30793#discussion_r1788273274)
non-blocking out-of-scope: Might be worth it to either deprecate true/false values for verbosity arguments or introduce an argument type specific to verbosity arguments to avoid skipping type checks for them.
💬 achow101 commented on pull request "Dialog for allowing the user to choose the change output when bumping a tx":
(https://github.com/bitcoin-core/gui/pull/700#discussion_r1788273291)
Done
(https://github.com/bitcoin-core/gui/pull/700#discussion_r1788273291)
Done
💬 achow101 commented on pull request "Dialog for allowing the user to choose the change output when bumping a tx":
(https://github.com/bitcoin-core/gui/pull/700#issuecomment-2394515614)
Rebased and fixed the build issue in the first commit.
(https://github.com/bitcoin-core/gui/pull/700#issuecomment-2394515614)
Rebased and fixed the build issue in the first commit.
💬 tdb3 commented on pull request "rpc: add getorphantxs":
(https://github.com/bitcoin/bitcoin/pull/30793#discussion_r1788275942)
I would support either, with the latter being my preference (at least initially/in transition)
(https://github.com/bitcoin/bitcoin/pull/30793#discussion_r1788275942)
I would support either, with the latter being my preference (at least initially/in transition)
💬 0xB10C commented on issue "Prioritize processing of peers based on their CPU usage":
(https://github.com/bitcoin/bitcoin/issues/31033#issuecomment-2394709587)
Interesting idea - I remember that we briefly chatted about this a while ago.
How does this behave during e.g. IBD when a "good" peer sends us a bunch of blocks and we spent a lot of CPU time validating them? Does something like this need to be excluded? I guess it's similar for peers that constantly send us transactions we don't know about yet (maybe because they are just better connected or they are broadcasting them) - not sure if we want to process their messages slower.
(https://github.com/bitcoin/bitcoin/issues/31033#issuecomment-2394709587)
Interesting idea - I remember that we briefly chatted about this a while ago.
How does this behave during e.g. IBD when a "good" peer sends us a bunch of blocks and we spent a lot of CPU time validating them? Does something like this need to be excluded? I guess it's similar for peers that constantly send us transactions we don't know about yet (maybe because they are just better connected or they are broadcasting them) - not sure if we want to process their messages slower.
💬 hodlinator commented on pull request "descriptor: Add proper Clone function to miniscript::Node":
(https://github.com/bitcoin/bitcoin/pull/30866#discussion_r1788379490)
The PR description asserts:
> Multipath descriptors requires performing a deep copy
Would be happy if you cared to add an elaboration on why that is.
It seemed to me like it should be safe to just have another `shared_ptr` point to the same `const Node`. Unless something on the outside could own a non-const reference into the node hierarchy and mutate it that way? In that case maybe it would be more robust for the `MiniscriptDescriptor`-ctor to be the one ensuring it holds a unique refere
...
(https://github.com/bitcoin/bitcoin/pull/30866#discussion_r1788379490)
The PR description asserts:
> Multipath descriptors requires performing a deep copy
Would be happy if you cared to add an elaboration on why that is.
It seemed to me like it should be safe to just have another `shared_ptr` point to the same `const Node`. Unless something on the outside could own a non-const reference into the node hierarchy and mutate it that way? In that case maybe it would be more robust for the `MiniscriptDescriptor`-ctor to be the one ensuring it holds a unique refere
...
💬 russeree commented on pull request "Enhanced error messages for invalid network prefix during address parsing.":
(https://github.com/bitcoin/bitcoin/pull/27260#discussion_r1788394254)
> [7478ac6](https://github.com/bitcoin/bitcoin/commit/7478ac6af922c302e292ace7f61e6eaa461da727): this change belongs in the previous commit [07f9733](https://github.com/bitcoin/bitcoin/commit/07f9733e6022d77d755321914a2a2a1dd7ce0d59)
>
> (One way to move it there is by doing `git rebase -i HEAD~3`, setting "e" for the first commit, and then add these lines. Then you do a `git commit -a --amend` to update the first commit. Then `git rebase --continue`, which should automatically adjust the sec
...
(https://github.com/bitcoin/bitcoin/pull/27260#discussion_r1788394254)
> [7478ac6](https://github.com/bitcoin/bitcoin/commit/7478ac6af922c302e292ace7f61e6eaa461da727): this change belongs in the previous commit [07f9733](https://github.com/bitcoin/bitcoin/commit/07f9733e6022d77d755321914a2a2a1dd7ce0d59)
>
> (One way to move it there is by doing `git rebase -i HEAD~3`, setting "e" for the first commit, and then add these lines. Then you do a `git commit -a --amend` to update the first commit. Then `git rebase --continue`, which should automatically adjust the sec
...
💬 russeree commented on pull request "Enhanced error messages for invalid network prefix during address parsing.":
(https://github.com/bitcoin/bitcoin/pull/27260#issuecomment-2394760664)
> Re last commit message, you can leave this out:
>
> > A complete refactor of the previous pull to simplify logic and reduce the
> > chance of technical debt.
This has been removed.
(https://github.com/bitcoin/bitcoin/pull/27260#issuecomment-2394760664)
> Re last commit message, you can leave this out:
>
> > A complete refactor of the previous pull to simplify logic and reduce the
> > chance of technical debt.
This has been removed.
💬 fjahr commented on issue "assumeutxo: not syncing the snapshot chainstate":
(https://github.com/bitcoin/bitcoin/issues/30971#issuecomment-2394768306)
> If this gets to the point that the snapshot load completes, I'll close this.
I don't think we have to close this right away, we can keep this open for a few more weeks to see if this issue occurs for other users. Having an open issue already makes it easier for people to engage. But did loading of the snapshot actually complete eventually @fanquake ?
I am currently looking into the obfuscation key because I find it suspicious that it changes for `chainstate_snapshot`. From my understand
...
(https://github.com/bitcoin/bitcoin/issues/30971#issuecomment-2394768306)
> If this gets to the point that the snapshot load completes, I'll close this.
I don't think we have to close this right away, we can keep this open for a few more weeks to see if this issue occurs for other users. Having an open issue already makes it easier for people to engage. But did loading of the snapshot actually complete eventually @fanquake ?
I am currently looking into the obfuscation key because I find it suspicious that it changes for `chainstate_snapshot`. From my understand
...
🤔 promag reviewed a pull request: "qt6: Handle different signatures of `QANEF::nativeEventFilter`"
(https://github.com/bitcoin-core/gui/pull/840#pullrequestreview-2349357223)
I think small changes to make code compatible with both versions are fine until support for qt5 is dropped.
Code review ACK 80761afced12c774a1768fb27a3975d456981ae0.
Alternative is to use type alias
```cpp
#if (QT_VERSION >= QT_VERSION_CHECK(6, 0, 0))
using result_t = qintptr*;
#else
using result_t = long*;
#endif
(https://github.com/bitcoin-core/gui/pull/840#pullrequestreview-2349357223)
I think small changes to make code compatible with both versions are fine until support for qt5 is dropped.
Code review ACK 80761afced12c774a1768fb27a3975d456981ae0.
Alternative is to use type alias
```cpp
#if (QT_VERSION >= QT_VERSION_CHECK(6, 0, 0))
using result_t = qintptr*;
#else
using result_t = long*;
#endif