💬 vasild commented on pull request "net: Fix Discover() not running when using -bind=0.0.0.0:port":
(https://github.com/bitcoin/bitcoin/pull/32757#discussion_r2333538296)
If you retouch, this comment is also outdated and needs a tweak, for example:
```suggestion
# We need to bind to a routable address for this test to exercise the relevant code
# and also must have another routable address. Those addresses must be on an interface
# that is UP and is not a loopback interface (IFF_LOOPBACK).
```
(https://github.com/bitcoin/bitcoin/pull/32757#discussion_r2333538296)
If you retouch, this comment is also outdated and needs a tweak, for example:
```suggestion
# We need to bind to a routable address for this test to exercise the relevant code
# and also must have another routable address. Those addresses must be on an interface
# that is UP and is not a loopback interface (IFF_LOOPBACK).
```
⚠️ ziggie1984 opened an issue: "Difficulty in reliably mapping errors from Bitcoin Core due to unstable error codes and messages"
(https://github.com/bitcoin/bitcoin/issues/33350)
### Please describe the feature you'd like to see added.
It's becoming increasingly difficult to reliably map and handle errors originating from Bitcoin Core. The primary challenge stems from the frequent changes in both the error codes and the descriptive error text.
Problem Description
Our current error-handling logic relies on parsing the responses from Bitcoin Core. However, this approach is fragile for two main reasons:
Unstable Error Codes: The numerical error codes returned by Bitcoin
...
(https://github.com/bitcoin/bitcoin/issues/33350)
### Please describe the feature you'd like to see added.
It's becoming increasingly difficult to reliably map and handle errors originating from Bitcoin Core. The primary challenge stems from the frequent changes in both the error codes and the descriptive error text.
Problem Description
Our current error-handling logic relies on parsing the responses from Bitcoin Core. However, this approach is fragile for two main reasons:
Unstable Error Codes: The numerical error codes returned by Bitcoin
...
💬 sipa commented on pull request "net: Prevent node from binding to the same `CService`":
(https://github.com/bitcoin/bitcoin/pull/33231#issuecomment-3270763797)
utACK 4d4789dffad55b96f1cb96b718cc6923f5344454
(https://github.com/bitcoin/bitcoin/pull/33231#issuecomment-3270763797)
utACK 4d4789dffad55b96f1cb96b718cc6923f5344454
💬 sipa commented on pull request "ci, iwyu: Treat warnings as errors for specific directories":
(https://github.com/bitcoin/bitcoin/pull/31308#issuecomment-3270768314)
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/31308#issuecomment-3270768314)
Concept ACK
👍 ryanofsky approved a pull request: "ci, iwyu: Treat warnings as errors for specific directories"
(https://github.com/bitcoin/bitcoin/pull/31308#pullrequestreview-3201650940)
Code review ACK 02d2b5a11c921ef71c971ee80eb3dfbc75c8cb0d. Just rebased and update tidy patch comment again since last review
(https://github.com/bitcoin/bitcoin/pull/31308#pullrequestreview-3201650940)
Code review ACK 02d2b5a11c921ef71c971ee80eb3dfbc75c8cb0d. Just rebased and update tidy patch comment again since last review
👍 ryanofsky approved a pull request: "ci: detect outbound internet traffic generated while running tests"
(https://github.com/bitcoin/bitcoin/pull/31349#pullrequestreview-3201679543)
Code review ACK 151edfaf78115402c29088dabc271c2b268102a5. Just rebased since last review and replaced node_init_tests fix CreateSock fix with natpmp=0 fix
(https://github.com/bitcoin/bitcoin/pull/31349#pullrequestreview-3201679543)
Code review ACK 151edfaf78115402c29088dabc271c2b268102a5. Just rebased since last review and replaced node_init_tests fix CreateSock fix with natpmp=0 fix
💬 rkrux commented on pull request "wallet: Be able to receive and spend inputs involving MuSig2 aggregate keys":
(https://github.com/bitcoin/bitcoin/pull/29675#discussion_r2333720544)
In https://github.com/bitcoin/bitcoin/commit/d23116987d19587746d392895c06f5c426c1d0d2 "test: Test MuSig2 in the wallet"
With the addition of the case where musig is both in keypath and spendpath, I think it would be nice to assert which spending path is triggered.
<details open>
<summary>Diff</summary>
```diff
diff --git a/test/functional/wallet_musig.py b/test/functional/wallet_musig.py
index b7f3cc9d96..63276b1eb7 100755
--- a/test/functional/wallet_musig.py
+++ b/test/functional
...
(https://github.com/bitcoin/bitcoin/pull/29675#discussion_r2333720544)
In https://github.com/bitcoin/bitcoin/commit/d23116987d19587746d392895c06f5c426c1d0d2 "test: Test MuSig2 in the wallet"
With the addition of the case where musig is both in keypath and spendpath, I think it would be nice to assert which spending path is triggered.
<details open>
<summary>Diff</summary>
```diff
diff --git a/test/functional/wallet_musig.py b/test/functional/wallet_musig.py
index b7f3cc9d96..63276b1eb7 100755
--- a/test/functional/wallet_musig.py
+++ b/test/functional
...
💬 rkrux commented on pull request "wallet: Be able to receive and spend inputs involving MuSig2 aggregate keys":
(https://github.com/bitcoin/bitcoin/pull/29675#discussion_r2333473293)
In d23116987d19587746d392895c06f5c426c1d0d2 "test: Test MuSig2 in the wallet"
Can also add the below test case that works right away where the musig is in both the key path spend and the script path spend - KP has all 3 keys in the musig, SP scripts have 2 partial keys in their musig each.
```python
self.do_test("tr(musig/*,{pk(partial keys diff musig-1/*),pk(partial keys diff musig-2/*)})}", "tr(musig($0,$1,$2)/<3;4>/*,{pk(musig($0,$1)/<5;6>/*),pk(musig($1,$2)/7/*)})")
```
(https://github.com/bitcoin/bitcoin/pull/29675#discussion_r2333473293)
In d23116987d19587746d392895c06f5c426c1d0d2 "test: Test MuSig2 in the wallet"
Can also add the below test case that works right away where the musig is in both the key path spend and the script path spend - KP has all 3 keys in the musig, SP scripts have 2 partial keys in their musig each.
```python
self.do_test("tr(musig/*,{pk(partial keys diff musig-1/*),pk(partial keys diff musig-2/*)})}", "tr(musig($0,$1,$2)/<3;4>/*,{pk(musig($0,$1)/<5;6>/*),pk(musig($1,$2)/7/*)})")
```
💬 instagibbs commented on issue "Difficulty in reliably mapping errors from Bitcoin Core due to unstable error codes and messages":
(https://github.com/bitcoin/bitcoin/issues/33350#issuecomment-3270872088)
Is there a easy to read table somewhere on how you are interpreting and using specific error codes / msgs and how your software is intended to respond to them? This seems like an twice a year issue, and as far as I know no one else is using the errors for non-debugging reasons, or they're too shy to complain about the breaks.
(https://github.com/bitcoin/bitcoin/issues/33350#issuecomment-3270872088)
Is there a easy to read table somewhere on how you are interpreting and using specific error codes / msgs and how your software is intended to respond to them? This seems like an twice a year issue, and as far as I know no one else is using the errors for non-debugging reasons, or they're too shy to complain about the breaks.
🤔 sipa reviewed a pull request: "multiprocess: Don't require bitcoin -m argument when IPC options are used"
(https://github.com/bitcoin/bitcoin/pull/33229#pullrequestreview-3201821957)
utACK f9685d6a1389938b0cceb31d9eef201ab3dd2e86
(https://github.com/bitcoin/bitcoin/pull/33229#pullrequestreview-3201821957)
utACK f9685d6a1389938b0cceb31d9eef201ab3dd2e86
💬 ryanofsky commented on pull request "multiprocess: Don't require bitcoin -m argument when IPC options are used":
(https://github.com/bitcoin/bitcoin/pull/33229#issuecomment-3270914430)
Updated the description and added this as a 30.0 miestone in case that makes sense. I'm not exactly sure how release milestones are used so we can remove it if this is inappropriate
(https://github.com/bitcoin/bitcoin/pull/33229#issuecomment-3270914430)
Updated the description and added this as a 30.0 miestone in case that makes sense. I'm not exactly sure how release milestones are used so we can remove it if this is inappropriate
💬 sipa commented on pull request "ci: detect outbound internet traffic generated while running tests":
(https://github.com/bitcoin/bitcoin/pull/31349#discussion_r2333792206)
This `INTERNET_TRAFFIC_EXPECTED` seems to be unused?
(https://github.com/bitcoin/bitcoin/pull/31349#discussion_r2333792206)
This `INTERNET_TRAFFIC_EXPECTED` seems to be unused?
💬 ziggie1984 commented on issue "Difficulty in reliably mapping errors from Bitcoin Core due to unstable error codes and messages":
(https://github.com/bitcoin/bitcoin/issues/33350#issuecomment-3270947536)
We don't have an easy table but I think for example this gives a good example:
https://github.com/lightningnetwork/lnd/blob/master/sweep/fee_bumper.go#L568-L578
In our fee bumper engine we only bump the fee of a transaction if the error is somehow related to an `insufficient fee` error, otherwise we drop this input from the sweeping system because problems like not having the correct signature or being already spent would cause us to never properly sweep this input.
What I am more interested
...
(https://github.com/bitcoin/bitcoin/issues/33350#issuecomment-3270947536)
We don't have an easy table but I think for example this gives a good example:
https://github.com/lightningnetwork/lnd/blob/master/sweep/fee_bumper.go#L568-L578
In our fee bumper engine we only bump the fee of a transaction if the error is somehow related to an `insufficient fee` error, otherwise we drop this input from the sweeping system because problems like not having the correct signature or being already spent would cause us to never properly sweep this input.
What I am more interested
...
🤔 stickies-v reviewed a pull request: "chore: compare against keys.size() in writeObject comma check"
(https://github.com/bitcoin/bitcoin/pull/33337#pullrequestreview-3201895990)
NACK, seems like busywork, no clear improvement
(https://github.com/bitcoin/bitcoin/pull/33337#pullrequestreview-3201895990)
NACK, seems like busywork, no clear improvement
💬 theStack commented on pull request "wallet: Be able to receive and spend inputs involving MuSig2 aggregate keys":
(https://github.com/bitcoin/bitcoin/pull/29675#discussion_r2333831377)
Note that the only musig API function that requires to use this context is [`secp256k1_musig_nonce_gen`](https://github.com/bitcoin-core/secp256k1/blob/36e76952cbf1cf54ddd2d8756cc31a486e2ba1d9/include/secp256k1_musig.h#L338), for all the others you can simply use the static one (`secp256k1_context_static`).
(https://github.com/bitcoin/bitcoin/pull/29675#discussion_r2333831377)
Note that the only musig API function that requires to use this context is [`secp256k1_musig_nonce_gen`](https://github.com/bitcoin-core/secp256k1/blob/36e76952cbf1cf54ddd2d8756cc31a486e2ba1d9/include/secp256k1_musig.h#L338), for all the others you can simply use the static one (`secp256k1_context_static`).
✅ fanquake closed a pull request: "chore: compare against keys.size() in writeObject comma check"
(https://github.com/bitcoin/bitcoin/pull/33337)
(https://github.com/bitcoin/bitcoin/pull/33337)
💬 sipa commented on pull request "Fix tiebreak when loading blocks from disk (and add tests for comparing chain ties)":
(https://github.com/bitcoin/bitcoin/pull/29640#discussion_r2333834031)
I think just setting the tip to 0 is fine. We don't actually know whether any of the tip's ancestors were activatable earlier than any of their potential competitors.
(https://github.com/bitcoin/bitcoin/pull/29640#discussion_r2333834031)
I think just setting the tip to 0 is fine. We don't actually know whether any of the tip's ancestors were activatable earlier than any of their potential competitors.
💬 instagibbs commented on issue "Difficulty in reliably mapping errors from Bitcoin Core due to unstable error codes and messages":
(https://github.com/bitcoin/bitcoin/issues/33350#issuecomment-3270998612)
Speaking only for myself since I'm more familiar with both LN (by means of CLN) and Bitcoin Core sides...
> Yes I am really surprised that people are not using those error codes to decided based on that what is wrong with their transaction what they for example trying to broadcast.
I think it's honestly quite rare. People will craft transactions using information gleaned Core (like mempool minfee or fee estimator) and proactively avoid violating it. The software can't do anything about other e
...
(https://github.com/bitcoin/bitcoin/issues/33350#issuecomment-3270998612)
Speaking only for myself since I'm more familiar with both LN (by means of CLN) and Bitcoin Core sides...
> Yes I am really surprised that people are not using those error codes to decided based on that what is wrong with their transaction what they for example trying to broadcast.
I think it's honestly quite rare. People will craft transactions using information gleaned Core (like mempool minfee or fee estimator) and proactively avoid violating it. The software can't do anything about other e
...
💬 fjahr commented on pull request "index: Fix coinstats overflow":
(https://github.com/bitcoin/bitcoin/pull/30469#issuecomment-3271072287)
> `getblockstats` gives the same result for both at height 913,859
Did you mean `gettxoutsetinfo` here? :)
(https://github.com/bitcoin/bitcoin/pull/30469#issuecomment-3271072287)
> `getblockstats` gives the same result for both at height 913,859
Did you mean `gettxoutsetinfo` here? :)
🤔 sipa reviewed a pull request: "Fix tiebreak when loading blocks from disk (and add tests for comparing chain ties)"
(https://github.com/bitcoin/bitcoin/pull/29640#pullrequestreview-3202025791)
utACK 0465574c127907df9b764055a585e8281bae8d1d
Happy with either the "whole active chain is set to nSequence 0" or the "only tip is set to nSequence 0" approaches.
(https://github.com/bitcoin/bitcoin/pull/29640#pullrequestreview-3202025791)
utACK 0465574c127907df9b764055a585e8281bae8d1d
Happy with either the "whole active chain is set to nSequence 0" or the "only tip is set to nSequence 0" approaches.