🤔 ismaelsadeeq reviewed a pull request: "test: use context managers and add file existence checks in feature_fee_estimation.py"
(https://github.com/bitcoin/bitcoin/pull/31030#pullrequestreview-2348455824)
Thanks for your interest in contributing to this project @AgusR7
I am unsure whether this PR meets the refactoring standards outlined in the contributing guidelines https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#refactoring, and I'm also not convinced that the advertised advantages justify the review effort. Therefore, I am tending towards NACK.
If you are still willing to contribute check out the [Good first issues](https://github.com/bitcoin/bitcoin/issues?q=is%3Aopen+
...
(https://github.com/bitcoin/bitcoin/pull/31030#pullrequestreview-2348455824)
Thanks for your interest in contributing to this project @AgusR7
I am unsure whether this PR meets the refactoring standards outlined in the contributing guidelines https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#refactoring, and I'm also not convinced that the advertised advantages justify the review effort. Therefore, I am tending towards NACK.
If you are still willing to contribute check out the [Good first issues](https://github.com/bitcoin/bitcoin/issues?q=is%3Aopen+
...
🤔 ismaelsadeeq reviewed a pull request: "cluster mempool: extend DepGraph functionality"
(https://github.com/bitcoin/bitcoin/pull/30857#pullrequestreview-2348460911)
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/30857#pullrequestreview-2348460911)
Concept ACK
📝 itornaza converted_to_draft a pull request: "depends: fix for llvm-ranlib (etc): 'No such file or directory' macOS 15.0"
(https://github.com/bitcoin/bitcoin/pull/30994)
This is an attempt to fix the issue https://github.com/bitcoin/bitcoin/issues/30978 .
To briefly summarize, @Sjors found that while compiling dependencies on macOS 15.0 the following "No such file or directory" are generated while these tools are installed.
```
$ cd depends
$ make
/bin/sh: command -v llvm-ranlib: No such file or directory
/bin/sh: command -v llvm-strip: No such file or directory
/bin/sh: command -v llvm-nm: No such file or directory
/bin/sh: command -v llvm-objdump:
...
(https://github.com/bitcoin/bitcoin/pull/30994)
This is an attempt to fix the issue https://github.com/bitcoin/bitcoin/issues/30978 .
To briefly summarize, @Sjors found that while compiling dependencies on macOS 15.0 the following "No such file or directory" are generated while these tools are installed.
```
$ cd depends
$ make
/bin/sh: command -v llvm-ranlib: No such file or directory
/bin/sh: command -v llvm-strip: No such file or directory
/bin/sh: command -v llvm-nm: No such file or directory
/bin/sh: command -v llvm-objdump:
...
💬 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
...