💬 fjahr commented on pull request "Rollback for dumptxoutset without invalidating blocks":
(https://github.com/bitcoin/bitcoin/pull/33477#issuecomment-3330760975)
cc @Sjors since you were asking for this approach a few times :)
(https://github.com/bitcoin/bitcoin/pull/33477#issuecomment-3330760975)
cc @Sjors since you were asking for this approach a few times :)
💬 ryanofsky commented on issue "`bitcoin-node` is unkillable after mining IPC connection is established":
(https://github.com/bitcoin/bitcoin/issues/33463#issuecomment-3330765943)
Have been trying to reproduce this bug in python by calling waitTipChanged followed by a shutdown but so far no luck.
With no `await asyncio.sleep` after `waitTipChanged` and before `stop_node` I get a "peer disconnected" error. With `await asyncio.sleep(0)` I get a "broken pipe" error, and with longer sleeps I see a nullopt BlockRef returned from waitTipChanged().
These results are all expected, and I didn't see a way to make the node hang on shutdown. Changes made to interface_ipc.py are sh
...
(https://github.com/bitcoin/bitcoin/issues/33463#issuecomment-3330765943)
Have been trying to reproduce this bug in python by calling waitTipChanged followed by a shutdown but so far no luck.
With no `await asyncio.sleep` after `waitTipChanged` and before `stop_node` I get a "peer disconnected" error. With `await asyncio.sleep(0)` I get a "broken pipe" error, and with longer sleeps I see a nullopt BlockRef returned from waitTipChanged().
These results are all expected, and I didn't see a way to make the node hang on shutdown. Changes made to interface_ipc.py are sh
...
🤔 mzumsande reviewed a pull request: "cli: Handle arguments that can be either JSON or string"
(https://github.com/bitcoin/bitcoin/pull/33230#pullrequestreview-3264798788)
Code Review ACK df67bb6fd84c393eaf00f19074085ee080546bd3
Possible additional changes or follow-ups:
- Release note (so that users know that this is possible now)
- Adjust doc examples (https://github.com/bitcoin/bitcoin/pull/33230#issuecomment-3216227775)
I am not really a fan of multi-type args that require workarounds like this - e.g. I see no reason `getblockstats` would support a height while `getblock` doesn't - but that ship has sailed.
(https://github.com/bitcoin/bitcoin/pull/33230#pullrequestreview-3264798788)
Code Review ACK df67bb6fd84c393eaf00f19074085ee080546bd3
Possible additional changes or follow-ups:
- Release note (so that users know that this is possible now)
- Adjust doc examples (https://github.com/bitcoin/bitcoin/pull/33230#issuecomment-3216227775)
I am not really a fan of multi-type args that require workarounds like this - e.g. I see no reason `getblockstats` would support a height while `getblock` doesn't - but that ship has sailed.
💬 plebhash commented on issue "`bitcoin-node` is unkillable after mining IPC connection is established":
(https://github.com/bitcoin/bitcoin/issues/33463#issuecomment-3330793682)
still haven't found the time for the stack trace, busy with Sv2 release... apologies for that
but it's pretty much gone on my side
so I'm happy if we close this, or I can also share stack trace later
(https://github.com/bitcoin/bitcoin/issues/33463#issuecomment-3330793682)
still haven't found the time for the stack trace, busy with Sv2 release... apologies for that
but it's pretty much gone on my side
so I'm happy if we close this, or I can also share stack trace later
💬 w0xlt commented on pull request "wallet: Add `exportwatchonlywallet` RPC to export a watchonly version of a wallet":
(https://github.com/bitcoin/bitcoin/pull/32489#discussion_r2377142018)
The assertion below (same as found in `wallet_migration.py`) fails.
Is this expected ?
```suggestion
# Assert no rescan occurs when restoring the exported watch-only wallet.
with self.online.assert_debug_log(unexpected_msgs=["Rescanning"]):
self.online.restorewallet(export_name, res["exported_file"])
```
(https://github.com/bitcoin/bitcoin/pull/32489#discussion_r2377142018)
The assertion below (same as found in `wallet_migration.py`) fails.
Is this expected ?
```suggestion
# Assert no rescan occurs when restoring the exported watch-only wallet.
with self.online.assert_debug_log(unexpected_msgs=["Rescanning"]):
self.online.restorewallet(export_name, res["exported_file"])
```
💬 alfredopalhares commented on pull request "docs: Undeprecate datacarrier and datacarriersize configuration options":
(https://github.com/bitcoin/bitcoin/pull/33453#issuecomment-3330855441)
Concept ACK
Come to your senses devs, the users have spoken.
(https://github.com/bitcoin/bitcoin/pull/33453#issuecomment-3330855441)
Concept ACK
Come to your senses devs, the users have spoken.
💬 151henry151 commented on pull request "build: Move CMAKE_SKIP_INSTALL_RPATH from CMake to Guix script":
(https://github.com/bitcoin/bitcoin/pull/33470#discussion_r2377208338)
I think I've made the correct adjustments -- can you check [dd5c517](https://github.com/bitcoin/bitcoin/commit/dd5c517757f97b68f7eb07628222c958b47f742b) and let me know if this is what you had in mind?
(https://github.com/bitcoin/bitcoin/pull/33470#discussion_r2377208338)
I think I've made the correct adjustments -- can you check [dd5c517](https://github.com/bitcoin/bitcoin/commit/dd5c517757f97b68f7eb07628222c958b47f742b) and let me know if this is what you had in mind?
🤔 BrunoSampaioDev reviewed a pull request: "rpc: addpeeraddress: throw on invalid IP"
(https://github.com/bitcoin/bitcoin/pull/33430#pullrequestreview-3265078475)
LGTM
(https://github.com/bitcoin/bitcoin/pull/33430#pullrequestreview-3265078475)
LGTM
💬 l0rinc commented on pull request "log: reduce excessive "rolling back/forward" messages during block replay":
(https://github.com/bitcoin/bitcoin/pull/33443#discussion_r2377590716)
Good point, thanks, fixed.
(https://github.com/bitcoin/bitcoin/pull/33443#discussion_r2377590716)
Good point, thanks, fixed.
💬 l0rinc commented on pull request "log: reduce excessive "rolling back/forward" messages during block replay":
(https://github.com/bitcoin/bitcoin/pull/33443#discussion_r2377591955)
Indeed, pushed, added you as coauthor.
I will try to add some tests for this either in this PR or in a follow-up since it doesn't seem to have any coverage.
(https://github.com/bitcoin/bitcoin/pull/33443#discussion_r2377591955)
Indeed, pushed, added you as coauthor.
I will try to add some tests for this either in this PR or in a follow-up since it doesn't seem to have any coverage.
💬 amishhaa commented on pull request "contrib: fix for macOS deployment build failing on Qt translations even though it is optional.":
(https://github.com/bitcoin/bitcoin/pull/33358#issuecomment-3332019270)
@fanquake, sorry of the delay! fixed.
(https://github.com/bitcoin/bitcoin/pull/33358#issuecomment-3332019270)
@fanquake, sorry of the delay! fixed.
🤔 weezyb0893 reviewed a pull request: "util: Revert "common: Close non-std fds before exec in RunCommandJSON""
(https://github.com/bitcoin/bitcoin/pull/33063#pullrequestreview-3265576651)
Stop
(https://github.com/bitcoin/bitcoin/pull/33063#pullrequestreview-3265576651)
Stop
💬 waketraindev commented on pull request "docs: Undeprecate datacarrier and datacarriersize configuration options":
(https://github.com/bitcoin/bitcoin/pull/33453#issuecomment-3332170515)
This PR is more of an example of the brink foundation leveraging and rallying their members to get a nonsensical help text change pushed and backported for V30.
The change itself is senseless and should not be something accepted in any kind of engineering.
Don't know who this attempts to please but it's clearly group think and group push from Brink.
-1 and concept nack and sure minimize this
(https://github.com/bitcoin/bitcoin/pull/33453#issuecomment-3332170515)
This PR is more of an example of the brink foundation leveraging and rallying their members to get a nonsensical help text change pushed and backported for V30.
The change itself is senseless and should not be something accepted in any kind of engineering.
Don't know who this attempts to please but it's clearly group think and group push from Brink.
-1 and concept nack and sure minimize this
✅ maflcko closed an issue: "Cleanup CFeeRate constructor (sat/vB vs BTC/kvB)"
(https://github.com/bitcoin/bitcoin/issues/23129)
(https://github.com/bitcoin/bitcoin/issues/23129)
💬 maflcko commented on issue "Cleanup CFeeRate constructor (sat/vB vs BTC/kvB)":
(https://github.com/bitcoin/bitcoin/issues/23129#issuecomment-3332398249)
Closing for now. Seems fine to just organically change this, the next time the code is touched.
(https://github.com/bitcoin/bitcoin/issues/23129#issuecomment-3332398249)
Closing for now. Seems fine to just organically change this, the next time the code is touched.
💬 purpleKarrot commented on pull request "CPack":
(https://github.com/bitcoin/bitcoin/pull/33455#discussion_r2377948657)
CMake does not provide options for all NSIS attributes. For attributes that are strictly required, we may need to upstream support (see [example](https://gitlab.kitware.com/cmake/cmake/-/issues/27260)).
However, we should also reconsider whether deviating from the defaults is truly necessary.
The default behavior for `CRCCheck` is to verify the CRC, but it also allows users to disable the check via a command-line option to the installer, which can be useful for testing purposes. Setting `force
...
(https://github.com/bitcoin/bitcoin/pull/33455#discussion_r2377948657)
CMake does not provide options for all NSIS attributes. For attributes that are strictly required, we may need to upstream support (see [example](https://gitlab.kitware.com/cmake/cmake/-/issues/27260)).
However, we should also reconsider whether deviating from the defaults is truly necessary.
The default behavior for `CRCCheck` is to verify the CRC, but it also allows users to disable the check via a command-line option to the installer, which can be useful for testing purposes. Setting `force
...
⚠️ waketraindev opened an issue: "No way to clear command history in RPC console or reset the console without restarting the node"
(https://github.com/bitcoin-core/gui/issues/897)
### Issues, reports or feature requests related to the GUI should be opened directly on the GUI repo
- [x] I still think this issue should be opened here
### Report
No way to clear the command history of a RPC console. With the history remaining until client reset.
Please add a way to either clear the command history or fully reset the console.
Thanks in advance
(https://github.com/bitcoin-core/gui/issues/897)
### Issues, reports or feature requests related to the GUI should be opened directly on the GUI repo
- [x] I still think this issue should be opened here
### Report
No way to clear the command history of a RPC console. With the history remaining until client reset.
Please add a way to either clear the command history or fully reset the console.
Thanks in advance
⚠️ waketraindev opened an issue: "Ability to retrieve peer by peer id"
(https://github.com/bitcoin/bitcoin/issues/33478)
### Please describe the feature you'd like to see added.
All the debug logs with data pertaining net peers contain a peer id, yet there's no way to retrieve that peer info without depending on other utilities like jq
Please add the ability to retrieve that peer information based on peer id.
Thanks in advance
### Is your feature related to a problem, if so please describe it.
_No response_
### Describe the solution you'd like
_No response_
### Describe any alternatives you've considered
...
(https://github.com/bitcoin/bitcoin/issues/33478)
### Please describe the feature you'd like to see added.
All the debug logs with data pertaining net peers contain a peer id, yet there's no way to retrieve that peer info without depending on other utilities like jq
Please add the ability to retrieve that peer information based on peer id.
Thanks in advance
### Is your feature related to a problem, if so please describe it.
_No response_
### Describe the solution you'd like
_No response_
### Describe any alternatives you've considered
...
💬 maflcko commented on pull request "validation: fetch block inputs on parallel threads >10% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r2378064596)
It disables the auto-detection for all functional tests by default, which I can't really find a downside to. Also, it removes idle "spam" threads while debugging (gdb and other tools will display less script check threads), which also seems beneficial to have?
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r2378064596)
It disables the auto-detection for all functional tests by default, which I can't really find a downside to. Also, it removes idle "spam" threads while debugging (gdb and other tools will display less script check threads), which also seems beneficial to have?
💬 maflcko commented on pull request "Bugfix: QA: rpc_bind: Skip nonloopback test if no such address is found":
(https://github.com/bitcoin/bitcoin/pull/33433#discussion_r2378402591)
I don't understand this change. This line of code is only run when `not self.options.run_ipv4 and not self.options.run_ipv6`. The whole test is only run when `sum([self.options.run_ipv4, self.options.run_ipv6, self.options.run_nonloopback]) > 1`. So `self.options.run_nonloopback` must be set.
However, the previous check already guard this:
```py
if self.non_loopback_ip is None and self.options.run_nonloopback:
raise SkipTest("This test requires a non-loopback ip addre
...
(https://github.com/bitcoin/bitcoin/pull/33433#discussion_r2378402591)
I don't understand this change. This line of code is only run when `not self.options.run_ipv4 and not self.options.run_ipv6`. The whole test is only run when `sum([self.options.run_ipv4, self.options.run_ipv6, self.options.run_nonloopback]) > 1`. So `self.options.run_nonloopback` must be set.
However, the previous check already guard this:
```py
if self.non_loopback_ip is None and self.options.run_nonloopback:
raise SkipTest("This test requires a non-loopback ip addre
...