💬 maflcko commented on pull request "build: suggest -DENABLE_IPC=OFF when missing capnp":
(https://github.com/bitcoin/bitcoin/pull/33290#issuecomment-3249086372)
> @maflcko the log is very verbose, but in indeed useful, so I opened #33291.
for reference, this specific warning was also printed without the verbose log: https://cirrus-ci.com/task/4516741763563520?logs=ci#L894 :
```
...
[03:43:58.798] CMake Error at /usr/lib/x86_64-linux-gnu/cmake/CapnProto/CapnProtoTargets.cmake:213 (add_executable):
[03:43:58.798] add_executable cannot create imported target "CapnProto::capnpc_cpp"
[03:43:58.798] because another target with the same name alre
...
(https://github.com/bitcoin/bitcoin/pull/33290#issuecomment-3249086372)
> @maflcko the log is very verbose, but in indeed useful, so I opened #33291.
for reference, this specific warning was also printed without the verbose log: https://cirrus-ci.com/task/4516741763563520?logs=ci#L894 :
```
...
[03:43:58.798] CMake Error at /usr/lib/x86_64-linux-gnu/cmake/CapnProto/CapnProtoTargets.cmake:213 (add_executable):
[03:43:58.798] add_executable cannot create imported target "CapnProto::capnpc_cpp"
[03:43:58.798] because another target with the same name alre
...
💬 theStack commented on issue "tracing: test `interface_usdt_net.py` fails due to garbage message type in `net:outbound_message` tracepoint":
(https://github.com/bitcoin/bitcoin/issues/33246#issuecomment-3249097001)
> > ```
> > Arguments: -8@x21 8@x24 8@[sp, 136] 8@[x0] 8@x1 8@x2
> > ```
>
> Are these the same if you build with `-DAPPEND_CXXFLAGS='-fno-inline'`?
They are quite different on a `-fno-inline` build:
```
Name: outbound_message
Location: 0x000000000012ed28, Base: 0x000000000111c6d8, Semaphore: 0x00000000016594d0
Arguments: -8@x23 8@x24 8@x25 8@x26 8@x27 8@x0
```
(https://github.com/bitcoin/bitcoin/issues/33246#issuecomment-3249097001)
> > ```
> > Arguments: -8@x21 8@x24 8@[sp, 136] 8@[x0] 8@x1 8@x2
> > ```
>
> Are these the same if you build with `-DAPPEND_CXXFLAGS='-fno-inline'`?
They are quite different on a `-fno-inline` build:
```
Name: outbound_message
Location: 0x000000000012ed28, Base: 0x000000000111c6d8, Semaphore: 0x00000000016594d0
Arguments: -8@x23 8@x24 8@x25 8@x26 8@x27 8@x0
```
💬 Sjors commented on pull request "build: suggest -DENABLE_IPC=OFF when missing capnp":
(https://github.com/bitcoin/bitcoin/pull/33290#issuecomment-3249117834)
Using `find_program` instead of `find_package` as suggested by an AI overlord, seems to do the trick.
Another issue was that the depends build also threw this warning, which I added a check for.
(https://github.com/bitcoin/bitcoin/pull/33290#issuecomment-3249117834)
Using `find_program` instead of `find_package` as suggested by an AI overlord, seems to do the trick.
Another issue was that the depends build also threw this warning, which I added a check for.
👋 Sjors's pull request is ready for review: "build: suggest -DENABLE_IPC=OFF when missing capnp"
(https://github.com/bitcoin/bitcoin/pull/33290)
(https://github.com/bitcoin/bitcoin/pull/33290)
💬 jmoik commented on issue "build: secp256k1 warnings not turned into errors in MSAN job":
(https://github.com/bitcoin/bitcoin/issues/33284#issuecomment-3249143680)
I actually cannot reproduce the exact warning of this macro, not on clang or g++. But adding an explicitly undefined variable produces a warning and is properly turned into an error with -DWERROR=ON after adding the inheritance.
(https://github.com/bitcoin/bitcoin/issues/33284#issuecomment-3249143680)
I actually cannot reproduce the exact warning of this macro, not on clang or g++. But adding an explicitly undefined variable produces a warning and is properly turned into an error with -DWERROR=ON after adding the inheritance.
💬 stickies-v commented on pull request "cmake: make missing Python interpreter behaviour more explicit":
(https://github.com/bitcoin/bitcoin/pull/33278#discussion_r2318903859)
> If they are independent, maybe give them independent default values?
Done, thanks. I used the default `BUILD_TESTS` value because I suspect in most cases if people don't want to build the unit tests, they also don't want to build the functional tests, but perhaps if we do want that logic it's best to have a `BUILD_TESTS` that acts like an umbrella setting, with `cmake_dependent_options` `BUILD_UNIT_TESTS` and `BUILD_FUNCTIONAL_TESTS`. That seems out of scope for this PR, so I've just change
...
(https://github.com/bitcoin/bitcoin/pull/33278#discussion_r2318903859)
> If they are independent, maybe give them independent default values?
Done, thanks. I used the default `BUILD_TESTS` value because I suspect in most cases if people don't want to build the unit tests, they also don't want to build the functional tests, but perhaps if we do want that logic it's best to have a `BUILD_TESTS` that acts like an umbrella setting, with `cmake_dependent_options` `BUILD_UNIT_TESTS` and `BUILD_FUNCTIONAL_TESTS`. That seems out of scope for this PR, so I've just change
...
💬 stickies-v commented on pull request "cmake: make missing Python interpreter behaviour more explicit":
(https://github.com/bitcoin/bitcoin/pull/33278#issuecomment-3249158729)
Force-pushed to default `BUILD_FUNCTIONAL_TESTS` to `ON` instead of to `BUILD_TESTS`, reducing behaviour change and addressing @purpleKarrot's [feedback](https://github.com/bitcoin/bitcoin/pull/33278#discussion_r2318148580).
(https://github.com/bitcoin/bitcoin/pull/33278#issuecomment-3249158729)
Force-pushed to default `BUILD_FUNCTIONAL_TESTS` to `ON` instead of to `BUILD_TESTS`, reducing behaviour change and addressing @purpleKarrot's [feedback](https://github.com/bitcoin/bitcoin/pull/33278#discussion_r2318148580).
👋 fanquake's pull request is ready for review: "[29.x] finalise v29.1"
(https://github.com/bitcoin/bitcoin/pull/33271)
(https://github.com/bitcoin/bitcoin/pull/33271)
💬 0xB10C commented on issue "tracing: test `interface_usdt_net.py` fails due to garbage message type in `net:outbound_message` tracepoint":
(https://github.com/bitcoin/bitcoin/issues/33246#issuecomment-3249195811)
These arguments translate to the `net:outbound_message` tracepoint arguments.
| arg | broken | `-fno-inline` working |
|-------------------------------------------|-------------|-----------------------|
| `pnode->GetId()` | -8@x21 | -8@x23 |
| `pnode->m_addr_name.c_str()` | 8@x24 | 8@x24 |
| `pnode->ConnectionTypeAsString().c_str()` | 8@[sp, 136] | 8@x25 |
|
...
(https://github.com/bitcoin/bitcoin/issues/33246#issuecomment-3249195811)
These arguments translate to the `net:outbound_message` tracepoint arguments.
| arg | broken | `-fno-inline` working |
|-------------------------------------------|-------------|-----------------------|
| `pnode->GetId()` | -8@x21 | -8@x23 |
| `pnode->m_addr_name.c_str()` | 8@x24 | 8@x24 |
| `pnode->ConnectionTypeAsString().c_str()` | 8@[sp, 136] | 8@x25 |
|
...
🤔 rkrux reviewed a pull request: "wallet: Identify transactions spending 0-value outputs, and add tests for anchor outputs in a wallet"
(https://github.com/bitcoin/bitcoin/pull/33268#pullrequestreview-3180181577)
Concept ACK 69eeb11bc585e92a3b56811c3251f13dba05aa5f
Agree with the intent. Nit in PR description because specifically `IsFromMe` is updated in the PR:
```diff
- One of the ways that the wallet would determine if a transaction belonged to the wallet was by checking
+ One of the ways that the wallet would determine if a transaction was sent from the wallet was by checking
```
(https://github.com/bitcoin/bitcoin/pull/33268#pullrequestreview-3180181577)
Concept ACK 69eeb11bc585e92a3b56811c3251f13dba05aa5f
Agree with the intent. Nit in PR description because specifically `IsFromMe` is updated in the PR:
```diff
- One of the ways that the wallet would determine if a transaction belonged to the wallet was by checking
+ One of the ways that the wallet would determine if a transaction was sent from the wallet was by checking
```
💬 rkrux commented on pull request "wallet: Identify transactions spending 0-value outputs, and add tests for anchor outputs in a wallet":
(https://github.com/bitcoin/bitcoin/pull/33268#discussion_r2318702489)
In 69eeb11bc585e92a3b56811c3251f13dba05aa5f "wallet: Add m_cached_from_me to cache "from me" status"
Maybe switch the order of the last 2 commits so that this commit also has coverage?
(https://github.com/bitcoin/bitcoin/pull/33268#discussion_r2318702489)
In 69eeb11bc585e92a3b56811c3251f13dba05aa5f "wallet: Add m_cached_from_me to cache "from me" status"
Maybe switch the order of the last 2 commits so that this commit also has coverage?
💬 rkrux commented on pull request "wallet: Identify transactions spending 0-value outputs, and add tests for anchor outputs in a wallet":
(https://github.com/bitcoin/bitcoin/pull/33268#discussion_r2318698691)
In 009b273df8ad8a7470276a31f3cb900e480aa99b "test: Test wallet 'from me' status change"
Are these necessary? The test passes without them even with a re-scan.
```diff
if confirm:
self.generate(self.nodes[0], 1, sync_fun=self.no_op)
- # Mock time forward and generate blocks so that the import does not rescan the transaction
- self.nodes[0].setmocktime(int(time.time()) + MAX_FUTURE_BLOCK_TIME + 1)
- self.generate(s
...
(https://github.com/bitcoin/bitcoin/pull/33268#discussion_r2318698691)
In 009b273df8ad8a7470276a31f3cb900e480aa99b "test: Test wallet 'from me' status change"
Are these necessary? The test passes without them even with a re-scan.
```diff
if confirm:
self.generate(self.nodes[0], 1, sync_fun=self.no_op)
- # Mock time forward and generate blocks so that the import does not rescan the transaction
- self.nodes[0].setmocktime(int(time.time()) + MAX_FUTURE_BLOCK_TIME + 1)
- self.generate(s
...
💬 rkrux commented on pull request "wallet: Identify transactions spending 0-value outputs, and add tests for anchor outputs in a wallet":
(https://github.com/bitcoin/bitcoin/pull/33268#discussion_r2318656200)
In 009b273df8ad8a7470276a31f3cb900e480aa99b "test: Test wallet 'from me' status change"
Probably can also assert on any detail entry being in send category after the descriptor is imported in the concerned wallet and not so before the import. Makes testing more explicit imho.
```diff
diff --git a/test/functional/wallet_listtransactions.py b/test/functional/wallet_listtransactions.py
index a19a1a32b1..0c60d4839d 100755
--- a/test/functional/wallet_listtransactions.py
+++ b/test/function
...
(https://github.com/bitcoin/bitcoin/pull/33268#discussion_r2318656200)
In 009b273df8ad8a7470276a31f3cb900e480aa99b "test: Test wallet 'from me' status change"
Probably can also assert on any detail entry being in send category after the descriptor is imported in the concerned wallet and not so before the import. Makes testing more explicit imho.
```diff
diff --git a/test/functional/wallet_listtransactions.py b/test/functional/wallet_listtransactions.py
index a19a1a32b1..0c60d4839d 100755
--- a/test/functional/wallet_listtransactions.py
+++ b/test/function
...
💬 rkrux commented on pull request "wallet: Identify transactions spending 0-value outputs, and add tests for anchor outputs in a wallet":
(https://github.com/bitcoin/bitcoin/pull/33268#discussion_r2318929377)
In https://github.com/bitcoin/bitcoin/commit/69eeb11bc585e92a3b56811c3251f13dba05aa5f "wallet: Add m_cached_from_me to cache "from me" status"
nit to shorten:
```diff
diff --git a/src/wallet/receive.cpp b/src/wallet/receive.cpp
index 4ed98c6d55..13a25b3632 100644
--- a/src/wallet/receive.cpp
+++ b/src/wallet/receive.cpp
@@ -195,8 +195,9 @@ void CachedTxGetAmounts(const CWallet& wallet, const CWalletTx& wtx,
bool CachedTxIsFromMe(const CWallet& wallet, const CWalletTx& wtx)
{
-
...
(https://github.com/bitcoin/bitcoin/pull/33268#discussion_r2318929377)
In https://github.com/bitcoin/bitcoin/commit/69eeb11bc585e92a3b56811c3251f13dba05aa5f "wallet: Add m_cached_from_me to cache "from me" status"
nit to shorten:
```diff
diff --git a/src/wallet/receive.cpp b/src/wallet/receive.cpp
index 4ed98c6d55..13a25b3632 100644
--- a/src/wallet/receive.cpp
+++ b/src/wallet/receive.cpp
@@ -195,8 +195,9 @@ void CachedTxGetAmounts(const CWallet& wallet, const CWalletTx& wtx,
bool CachedTxIsFromMe(const CWallet& wallet, const CWalletTx& wtx)
{
-
...
💬 rkrux commented on pull request "wallet: Identify transactions spending 0-value outputs, and add tests for anchor outputs in a wallet":
(https://github.com/bitcoin/bitcoin/pull/33268#discussion_r2318869615)
In https://github.com/bitcoin/bitcoin/commit/1cd57c67b10a771d2eb1c2324b241533c0b1f287 "test: Add a test for anchor outputs in the wallet"
While passing the `inputs` in this RPC is fine, I feel another frequently used way of calling `sendall` RPC could be to not pass the `inputs` in the request, in which case the RPC throws `Internal bug detected`.
```zsh
test_framework.authproxy.JSONRPCException: Internal bug detected: output.input_bytes > 0
wallet/rpc/spend.cpp:1524 (operator())
Bitcoi
...
(https://github.com/bitcoin/bitcoin/pull/33268#discussion_r2318869615)
In https://github.com/bitcoin/bitcoin/commit/1cd57c67b10a771d2eb1c2324b241533c0b1f287 "test: Add a test for anchor outputs in the wallet"
While passing the `inputs` in this RPC is fine, I feel another frequently used way of calling `sendall` RPC could be to not pass the `inputs` in the request, in which case the RPC throws `Internal bug detected`.
```zsh
test_framework.authproxy.JSONRPCException: Internal bug detected: output.input_bytes > 0
wallet/rpc/spend.cpp:1524 (operator())
Bitcoi
...
💬 rkrux commented on pull request "wallet: Identify transactions spending 0-value outputs, and add tests for anchor outputs in a wallet":
(https://github.com/bitcoin/bitcoin/pull/33268#discussion_r2318733172)
In 1cd57c67b10a771d2eb1c2324b241533c0b1f287 "test: Add a test for anchor outputs in the wallet"
supernit:
```diff
index 1d1435145a..a47f3c6b3c 100755
--- a/test/functional/wallet_anchor.py
+++ b/test/functional/wallet_anchor.py
@@ -71,13 +71,13 @@ class WalletAnchorTest(BitcoinTestFramework):
assert_equal(utxos[0]["txid"], anchor_txid)
assert_equal(utxos[0]["address"], ANCHOR_ADDRESS)
assert_equal(utxos[0]["amount"], 0)
- wallet.gettransaction(anch
...
(https://github.com/bitcoin/bitcoin/pull/33268#discussion_r2318733172)
In 1cd57c67b10a771d2eb1c2324b241533c0b1f287 "test: Add a test for anchor outputs in the wallet"
supernit:
```diff
index 1d1435145a..a47f3c6b3c 100755
--- a/test/functional/wallet_anchor.py
+++ b/test/functional/wallet_anchor.py
@@ -71,13 +71,13 @@ class WalletAnchorTest(BitcoinTestFramework):
assert_equal(utxos[0]["txid"], anchor_txid)
assert_equal(utxos[0]["address"], ANCHOR_ADDRESS)
assert_equal(utxos[0]["amount"], 0)
- wallet.gettransaction(anch
...
💬 rkrux commented on pull request "wallet: Identify transactions spending 0-value outputs, and add tests for anchor outputs in a wallet":
(https://github.com/bitcoin/bitcoin/pull/33268#discussion_r2318809119)
In 1cd57c67b10a771d2eb1c2324b241533c0b1f287 "test: Add a test for anchor outputs in the wallet"
There is a bit of duplication in all 3 tests. Maybe can remove this test altogether and put some of its functionality in the below test.
```diff
diff --git a/test/functional/wallet_anchor.py b/test/functional/wallet_anchor.py
index 1d1435145a..e6872deba0 100755
--- a/test/functional/wallet_anchor.py
+++ b/test/functional/wallet_anchor.py
@@ -79,40 +79,22 @@ class WalletAnchorTest(BitcoinTestF
...
(https://github.com/bitcoin/bitcoin/pull/33268#discussion_r2318809119)
In 1cd57c67b10a771d2eb1c2324b241533c0b1f287 "test: Add a test for anchor outputs in the wallet"
There is a bit of duplication in all 3 tests. Maybe can remove this test altogether and put some of its functionality in the below test.
```diff
diff --git a/test/functional/wallet_anchor.py b/test/functional/wallet_anchor.py
index 1d1435145a..e6872deba0 100755
--- a/test/functional/wallet_anchor.py
+++ b/test/functional/wallet_anchor.py
@@ -79,40 +79,22 @@ class WalletAnchorTest(BitcoinTestF
...
💬 rkrux commented on pull request "wallet: Identify transactions spending 0-value outputs, and add tests for anchor outputs in a wallet":
(https://github.com/bitcoin/bitcoin/pull/33268#discussion_r2318709095)
In 1cd57c67b10a771d2eb1c2324b241533c0b1f287 "test: Add a test for anchor outputs in the wallet"
Nit: Probably can stay in the util file where `PAY_TO_ANCHOR` is defined.
(https://github.com/bitcoin/bitcoin/pull/33268#discussion_r2318709095)
In 1cd57c67b10a771d2eb1c2324b241533c0b1f287 "test: Add a test for anchor outputs in the wallet"
Nit: Probably can stay in the util file where `PAY_TO_ANCHOR` is defined.
💬 0xB10C commented on issue "tracing: test `interface_usdt_net.py` fails due to garbage message type in `net:outbound_message` tracepoint":
(https://github.com/bitcoin/bitcoin/issues/33246#issuecomment-3249215214)
As per https://github.com/iovisor/bcc/blob/791bf81ad9ec18ce2899f79509e4de96ddc21e9d/src/cc/usdt/usdt_args.cc#L228-L231, bcc on `aarch64` claims to support parsing `8@[x0]` via `[-]<size>@[<reg>]`.
```c
// Support the following argument patterns:
// [-]<size>@<value>, [-]<size>@<reg>, [-]<size>@[<reg>], or
// [-]<size>@[<reg>,<offset>]
// [-]<size>@[<reg>,<index_reg>]
```
(https://github.com/bitcoin/bitcoin/issues/33246#issuecomment-3249215214)
As per https://github.com/iovisor/bcc/blob/791bf81ad9ec18ce2899f79509e4de96ddc21e9d/src/cc/usdt/usdt_args.cc#L228-L231, bcc on `aarch64` claims to support parsing `8@[x0]` via `[-]<size>@[<reg>]`.
```c
// Support the following argument patterns:
// [-]<size>@<value>, [-]<size>@<reg>, [-]<size>@[<reg>], or
// [-]<size>@[<reg>,<offset>]
// [-]<size>@[<reg>,<index_reg>]
```
🤔 janb84 reviewed a pull request: "build: suggest -DENABLE_IPC=OFF when missing capnp"
(https://github.com/bitcoin/bitcoin/pull/33290#pullrequestreview-3180629451)
ACK f39a782ba8629bbca1587229f91ba2318d6b765c
PR adds warning text to cmake configure step if CAPNP is not installed. The CAPNP dependency is new, and this warning helps people that do not read the Releasenotes how to solve the error.
Although I do not recommend to upgrade without reading the Release notes / changelog, this warning can reduce the number of issues and guides (non-technical) users to avoid errors .
- code review ✅
- build with and without capnp to test warning. ✅
<de
...
(https://github.com/bitcoin/bitcoin/pull/33290#pullrequestreview-3180629451)
ACK f39a782ba8629bbca1587229f91ba2318d6b765c
PR adds warning text to cmake configure step if CAPNP is not installed. The CAPNP dependency is new, and this warning helps people that do not read the Releasenotes how to solve the error.
Although I do not recommend to upgrade without reading the Release notes / changelog, this warning can reduce the number of issues and guides (non-technical) users to avoid errors .
- code review ✅
- build with and without capnp to test warning. ✅
<de
...