👍 TheCharlatan approved a pull request: "multiprocess: Add -ipcbind option to bitcoin-node"
(https://github.com/bitcoin/bitcoin/pull/30509#pullrequestreview-2263113816)
ACK af24810eeed397c2fe5f61ef9769e1b7ee0ecdf9
This looks good to me. Thanks for following up on my earlier suggestions, I left a couple more. It would be nice to add some functional tests too, but that is easily done in a follow-up.
(https://github.com/bitcoin/bitcoin/pull/30509#pullrequestreview-2263113816)
ACK af24810eeed397c2fe5f61ef9769e1b7ee0ecdf9
This looks good to me. Thanks for following up on my earlier suggestions, I left a couple more. It would be nice to add some functional tests too, but that is easily done in a follow-up.
💬 TheCharlatan commented on pull request "multiprocess: Add -ipcbind option to bitcoin-node":
(https://github.com/bitcoin/bitcoin/pull/30509#discussion_r1733464897)
I think this should be added to the `rpc_help.py` tests.
(https://github.com/bitcoin/bitcoin/pull/30509#discussion_r1733464897)
I think this should be added to the `rpc_help.py` tests.
💬 TheCharlatan commented on pull request "multiprocess: Add -ipcbind option to bitcoin-node":
(https://github.com/bitcoin/bitcoin/pull/30509#discussion_r1733490705)
I think it would be good to sanity-check that invalid addresses make `bind` and `connect` throw:
```diff
diff --git a/src/test/ipc_test.cpp b/src/test/ipc_test.cpp
index f6addb2697..a5f6efbfb7 100644
--- a/src/test/ipc_test.cpp
+++ b/src/test/ipc_test.cpp
@@ -16,0 +17 @@
+#include <stdexcept>
@@ -121,0 +123,4 @@ void IpcSocketTest(const fs::path& datadir)
+ std::string invalid_bind{"invalid:"};
+ BOOST_CHECK_THROW(process->bind(datadir, "test_bitcoin", invalid_bind), std::invalid
...
(https://github.com/bitcoin/bitcoin/pull/30509#discussion_r1733490705)
I think it would be good to sanity-check that invalid addresses make `bind` and `connect` throw:
```diff
diff --git a/src/test/ipc_test.cpp b/src/test/ipc_test.cpp
index f6addb2697..a5f6efbfb7 100644
--- a/src/test/ipc_test.cpp
+++ b/src/test/ipc_test.cpp
@@ -16,0 +17 @@
+#include <stdexcept>
@@ -121,0 +123,4 @@ void IpcSocketTest(const fs::path& datadir)
+ std::string invalid_bind{"invalid:"};
+ BOOST_CHECK_THROW(process->bind(datadir, "test_bitcoin", invalid_bind), std::invalid
...
🚀 achow101 merged a pull request: "node: reduce unsafe uint256S usage"
(https://github.com/bitcoin/bitcoin/pull/30569)
(https://github.com/bitcoin/bitcoin/pull/30569)
💬 furszy commented on pull request "Bugfix: Ensure Atomicity in Wallet Settings Updates from Chain Interface":
(https://github.com/bitcoin/bitcoin/pull/30697#issuecomment-2313527490)
> Also note that this PR did not update Node::updateRwSetting, which now behaves more like Chain::overwriteRwSetting, and is theoretically vulnerable to the same race conditions as the ones addressed in this PR. Since this is currently only used by the GUI, triggering a race condition there seems less feasible though.
Yeah. I didn't touch it to avoid expanding this PR beyond what was necessary to fix the race. The GUI settings change events are executed sequentially on the main thread, so the
...
(https://github.com/bitcoin/bitcoin/pull/30697#issuecomment-2313527490)
> Also note that this PR did not update Node::updateRwSetting, which now behaves more like Chain::overwriteRwSetting, and is theoretically vulnerable to the same race conditions as the ones addressed in this PR. Since this is currently only used by the GUI, triggering a race condition there seems less feasible though.
Yeah. I didn't touch it to avoid expanding this PR beyond what was necessary to fix the race. The GUI settings change events are executed sequentially on the main thread, so the
...
💬 brunoerg commented on pull request "fuzz: fix timeout in `crypto_fschacha20poly1305`":
(https://github.com/bitcoin/bitcoin/pull/30725#issuecomment-2313592604)
> did you check if the timeout happens in crypto_aeadchacha20poly1305 too?
I think we did not have an issue with that yet, but anyway, I'll investigate it and follow-up.
(https://github.com/bitcoin/bitcoin/pull/30725#issuecomment-2313592604)
> did you check if the timeout happens in crypto_aeadchacha20poly1305 too?
I think we did not have an issue with that yet, but anyway, I'll investigate it and follow-up.
💬 hodlinator commented on pull request "test: Shut down framework cleanly on RPC connection failure":
(https://github.com/bitcoin/bitcoin/pull/30660#issuecomment-2313645385)
After more investigation into #30390, my impression is that since `TestNode.wait_for_rpc_connection()` ignores a bunch of exceptions, the knock-on exceptions when calling `stop()` and failing prior to this PR actually may sometimes provide a clue for why the RPC connection wasn't established in the first place.
That realization made me add 2 more commits here, which provide more information on the original exceptions being triggered and ignored inside `wait_for_rpc_connection`. Only fair sinc
...
(https://github.com/bitcoin/bitcoin/pull/30660#issuecomment-2313645385)
After more investigation into #30390, my impression is that since `TestNode.wait_for_rpc_connection()` ignores a bunch of exceptions, the knock-on exceptions when calling `stop()` and failing prior to this PR actually may sometimes provide a clue for why the RPC connection wasn't established in the first place.
That realization made me add 2 more commits here, which provide more information on the original exceptions being triggered and ignored inside `wait_for_rpc_connection`. Only fair sinc
...
💬 martinsaposnic commented on pull request "Use MiniWallet in functional test rpc_signrawtransactionwithkey.":
(https://github.com/bitcoin/bitcoin/pull/30701#discussion_r1733579735)
I noticed that MiniWallet's `send_to` method doesn't play nice with non-integer amounts. It's failing because CTxOut's `nValue` is an int, so when you pass a float, the `to_bytes` method throws an error.
I did a quick scan of the codebase:
We've got 21 uses of MiniWallet's `send_to` method on the tests (not counting this PR)
- 17 of these are already sending an integer
- 4 are explicitly doing int(amount) before passing it to `send_to`
I think fixing this is probably outside what we'r
...
(https://github.com/bitcoin/bitcoin/pull/30701#discussion_r1733579735)
I noticed that MiniWallet's `send_to` method doesn't play nice with non-integer amounts. It's failing because CTxOut's `nValue` is an int, so when you pass a float, the `to_bytes` method throws an error.
I did a quick scan of the codebase:
We've got 21 uses of MiniWallet's `send_to` method on the tests (not counting this PR)
- 17 of these are already sending an integer
- 4 are explicitly doing int(amount) before passing it to `send_to`
I think fixing this is probably outside what we'r
...
📝 jonatack opened a pull request: "rpc: add address_type field in getaddressinfo"
(https://github.com/bitcoin/bitcoin/pull/30727)
While creating and verifying fresh addresses today, noticed it would be helpful to see the address type in RPC getaddressinfo.
Also extracted repeated help docs to a utility helper.
Will add a release note if there are concept acks.
(https://github.com/bitcoin/bitcoin/pull/30727)
While creating and verifying fresh addresses today, noticed it would be helpful to see the address type in RPC getaddressinfo.
Also extracted repeated help docs to a utility helper.
Will add a release note if there are concept acks.
💬 jonatack commented on pull request "rpc: add address_type field in getaddressinfo":
(https://github.com/bitcoin/bitcoin/pull/30727#issuecomment-2313704286)
(Looking for Concept ACKs. Not sure this derives the output type correctly yet.)
(https://github.com/bitcoin/bitcoin/pull/30727#issuecomment-2313704286)
(Looking for Concept ACKs. Not sure this derives the output type correctly yet.)
💬 kevkevinpal commented on pull request "lint: Speed up and fix flake8 checks":
(https://github.com/bitcoin/bitcoin/pull/30723#discussion_r1733637530)
I am not sure how I feel about this `--preview` flag
According to their github docs in the [configuration section](https://github.com/astral-sh/ruff?tab=readme-ov-file#configuration)
> To opt in to the latest lint rules, formatter style changes, interface updates, and more, enable [preview mode](https://docs.astral.sh/ruff/rules/) by setting preview = true in your configuration file or passing --preview on the command line. Preview mode enables a collection of **unstable** features that
...
(https://github.com/bitcoin/bitcoin/pull/30723#discussion_r1733637530)
I am not sure how I feel about this `--preview` flag
According to their github docs in the [configuration section](https://github.com/astral-sh/ruff?tab=readme-ov-file#configuration)
> To opt in to the latest lint rules, formatter style changes, interface updates, and more, enable [preview mode](https://docs.astral.sh/ruff/rules/) by setting preview = true in your configuration file or passing --preview on the command line. Preview mode enables a collection of **unstable** features that
...
🤔 tdb3 reviewed a pull request: "rpc: add address_type field in getaddressinfo"
(https://github.com/bitcoin/bitcoin/pull/30727#pullrequestreview-2264669699)
Concept ACK
This seems like a useful addition.
```
$ src/bitcoin-cli getaddressinfo bc1p8k4v4xuz55dv49svzjg43qjxq2whur7ync9tm0xgl5t4wjl9ca9snxgmlt
{
"address": "bc1p8k4v4xuz55dv49svzjg43qjxq2whur7ync9tm0xgl5t4wjl9ca9snxgmlt",
"address_type": "bech32m",
"scriptPubKey": "51203daaca9b82a51aca960c1491588246029d7e0fc49e0abdbcc8fd17574be5c74b",
"ismine": false,
"solvable": false,
"iswatchonly": false,
"isscript": true,
"iswitness": true,
"witness_version": 1,
"witness
...
(https://github.com/bitcoin/bitcoin/pull/30727#pullrequestreview-2264669699)
Concept ACK
This seems like a useful addition.
```
$ src/bitcoin-cli getaddressinfo bc1p8k4v4xuz55dv49svzjg43qjxq2whur7ync9tm0xgl5t4wjl9ca9snxgmlt
{
"address": "bc1p8k4v4xuz55dv49svzjg43qjxq2whur7ync9tm0xgl5t4wjl9ca9snxgmlt",
"address_type": "bech32m",
"scriptPubKey": "51203daaca9b82a51aca960c1491588246029d7e0fc49e0abdbcc8fd17574be5c74b",
"ismine": false,
"solvable": false,
"iswatchonly": false,
"isscript": true,
"iswitness": true,
"witness_version": 1,
"witness
...
🤔 stratospher reviewed a pull request: "fuzz: fix timeout in `crypto_fschacha20poly1305`"
(https://github.com/bitcoin/bitcoin/pull/30725#pullrequestreview-2265091230)
ACK 8dec4e1. saw similar coverage stats (these are from different machines, saw more similar from same machine).
1. on branch
`#3974498 REDUCE cov: 544 ft: 3788 corp: 250/22Kb lim: 4096 exec/s: 124 rss: 538Mb L: 95/573 MS: 5 ChangeByte-ChangeByte-EraseBytes-ChangeBinInt-InsertByte-`
2. on master
`#3975366 REDUCE cov: 571 ft: 3993 corp: 279/51Kb lim: 4096 exec/s: 148 rss: 829Mb L: 3009/3818 MS: 1 EraseBytes-`
(https://github.com/bitcoin/bitcoin/pull/30725#pullrequestreview-2265091230)
ACK 8dec4e1. saw similar coverage stats (these are from different machines, saw more similar from same machine).
1. on branch
`#3974498 REDUCE cov: 544 ft: 3788 corp: 250/22Kb lim: 4096 exec/s: 124 rss: 538Mb L: 95/573 MS: 5 ChangeByte-ChangeByte-EraseBytes-ChangeBinInt-InsertByte-`
2. on master
`#3975366 REDUCE cov: 571 ft: 3993 corp: 279/51Kb lim: 4096 exec/s: 148 rss: 829Mb L: 3009/3818 MS: 1 EraseBytes-`
💬 maflcko commented on pull request "rpc: add address_type field in getaddressinfo":
(https://github.com/bitcoin/bitcoin/pull/30727#discussion_r1734040726)
Not sure about mapping the type to an enum that is only used to tell the wallet which descriptors to create. (It only appears as RPCArg, and I think using it in RPCResult is likely wrong). Also mapping p2sh-segwit to the "legacy" bin seems confusing and inconsistent with the input. So effectively this only differentiates between bech32m and bech32. This is fully redundant and less detailed than the already existing `witness_version` field.
(https://github.com/bitcoin/bitcoin/pull/30727#discussion_r1734040726)
Not sure about mapping the type to an enum that is only used to tell the wallet which descriptors to create. (It only appears as RPCArg, and I think using it in RPCResult is likely wrong). Also mapping p2sh-segwit to the "legacy" bin seems confusing and inconsistent with the input. So effectively this only differentiates between bech32m and bech32. This is fully redundant and less detailed than the already existing `witness_version` field.
✅ maflcko closed a pull request: "OP_CHECKTEMPLATEVERIFY, OP_CHECKSIGFROMSTACK(VERIFY), OP_INTERNALKEY validation (LNHANCE)"
(https://github.com/bitcoin/bitcoin/pull/29198)
(https://github.com/bitcoin/bitcoin/pull/29198)
💬 maflcko commented on pull request "OP_CHECKTEMPLATEVERIFY, OP_CHECKSIGFROMSTACK(VERIFY), OP_INTERNALKEY validation (LNHANCE)":
(https://github.com/bitcoin/bitcoin/pull/29198#issuecomment-2314387290)
> opening this PR against master may not really make sense all things considered. i don't suppose anyone is under the delusion that it will be merged.
Ok, closing for now
(https://github.com/bitcoin/bitcoin/pull/29198#issuecomment-2314387290)
> opening this PR against master may not really make sense all things considered. i don't suppose anyone is under the delusion that it will be merged.
Ok, closing for now
💬 hodlinator commented on pull request "refactor: Replace ParseHex with consteval ""_hex literals":
(https://github.com/bitcoin/bitcoin/pull/30377#issuecomment-2314476543)
So.. I have a rebase (1254e18e48e65032043bf688a1d33f41f8d539f8) on top of the now merged #29369 prepped that would fully resolve https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1722229379 and https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1719742146. It passed basic CI for the 3 platforms.
Changes are:
* 3 out of 5 replacements in **miniscript_tests.cpp** are moved to the scripted-diff commit.
* The new `ToScript` in **script_tests.cpp** can now use the more natural `{s
...
(https://github.com/bitcoin/bitcoin/pull/30377#issuecomment-2314476543)
So.. I have a rebase (1254e18e48e65032043bf688a1d33f41f8d539f8) on top of the now merged #29369 prepped that would fully resolve https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1722229379 and https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1719742146. It passed basic CI for the 3 platforms.
Changes are:
* 3 out of 5 replacements in **miniscript_tests.cpp** are moved to the scripted-diff commit.
* The new `ToScript` in **script_tests.cpp** can now use the more natural `{s
...
💬 maflcko commented on pull request "lint: Speed up and fix flake8 checks":
(https://github.com/bitcoin/bitcoin/pull/30723#discussion_r1734109145)
Yes, I am aware. However, the dependency is fully pinned, so if a feature changes, it will be noticed. This is no different from any of the other dependencies being pinned to avoid silently breaking updates.
However, it isn't needed, so I've removed it to restore the previous flags.
(https://github.com/bitcoin/bitcoin/pull/30723#discussion_r1734109145)
Yes, I am aware. However, the dependency is fully pinned, so if a feature changes, it will be noticed. This is no different from any of the other dependencies being pinned to avoid silently breaking updates.
However, it isn't needed, so I've removed it to restore the previous flags.
💬 maflcko commented on issue "b-msghand[4988] general protection fault":
(https://github.com/bitcoin/bitcoin/issues/30706#issuecomment-2314550483)
> Is there anything else I should do before next crash (if it ever happens), regarding debugging symbols? How to "tell" gdb that I have them? Thanks.
The idea was that you don't have to tell gdb before a crash, but that you can (even after the crash) convert the addr-stack to something readable.
However, I couldn't get that to work, so someone else will have to explain how it is supposed to work.
My next recommendation would be to re-compile with guix, but skip the split-debug.sh step,
...
(https://github.com/bitcoin/bitcoin/issues/30706#issuecomment-2314550483)
> Is there anything else I should do before next crash (if it ever happens), regarding debugging symbols? How to "tell" gdb that I have them? Thanks.
The idea was that you don't have to tell gdb before a crash, but that you can (even after the crash) convert the addr-stack to something readable.
However, I couldn't get that to work, so someone else will have to explain how it is supposed to work.
My next recommendation would be to re-compile with guix, but skip the split-debug.sh step,
...
💬 jonatack commented on pull request "rpc: add address_type field in getaddressinfo":
(https://github.com/bitcoin/bitcoin/pull/30727#discussion_r1734155198)
Yup still WIP. It would be nice to provide users with a one-to-one mapping between the input values, supplied to RPC getnewaddress `address_type` and the `-addresstype` and `-changetype` config options, and the same values returned on the other side. If it's feasible, for as you rightly point out, it currently isn't consistent. Pointers welcome.
(https://github.com/bitcoin/bitcoin/pull/30727#discussion_r1734155198)
Yup still WIP. It would be nice to provide users with a one-to-one mapping between the input values, supplied to RPC getnewaddress `address_type` and the `-addresstype` and `-changetype` config options, and the same values returned on the other side. If it's feasible, for as you rightly point out, it currently isn't consistent. Pointers welcome.