💬 ldionne commented on pull request "doc: Update documentation to include Clang/llvm based coverage report generation":
(https://github.com/bitcoin/bitcoin/pull/31933#discussion_r2001305336)
Sadly, I am not very familiar with how code coverage works in Clang (or other compilers), so I am not the best person to answer that question. However, as a general rule of thumb, I would encourage you to go with what the documentation says, and if you then notice shortcomings, report those as bugs against the project.
(https://github.com/bitcoin/bitcoin/pull/31933#discussion_r2001305336)
Sadly, I am not very familiar with how code coverage works in Clang (or other compilers), so I am not the best person to answer that question. However, as a general rule of thumb, I would encourage you to go with what the documentation says, and if you then notice shortcomings, report those as bugs against the project.
💬 remyers commented on pull request "wallet: add coin selection parameter `add_excess_to_recipient_position` for changeless txs with excess that would be added to fees":
(https://github.com/bitcoin/bitcoin/pull/30080#issuecomment-2733647933)
> This doesn't explain _why_ this parameter is needed. I'm not saying it's not, just that more explanation would be helpful.
You would want to set `max_excess` to something larger than `cost_of_change` if you want the utxos in your wallet to all be greater than `max_excess`.
For example, in the case of a Lightning node that is selling liquidity, you might know the smallest utxo you might sell is 10,000 sats + spend fees, so you could set `max_excess` to something like 15,000 sats. Now whe
...
(https://github.com/bitcoin/bitcoin/pull/30080#issuecomment-2733647933)
> This doesn't explain _why_ this parameter is needed. I'm not saying it's not, just that more explanation would be helpful.
You would want to set `max_excess` to something larger than `cost_of_change` if you want the utxos in your wallet to all be greater than `max_excess`.
For example, in the case of a Lightning node that is selling liquidity, you might know the smallest utxo you might sell is 10,000 sats + spend fees, so you could set `max_excess` to something like 15,000 sats. Now whe
...
💬 maflcko commented on issue "intermittent issue in p2p_orphan_handling.py":
(https://github.com/bitcoin/bitcoin/issues/31700#issuecomment-2733704193)
@tnndbtc Thank you for the exact diff to reproduce. This makes it easier to test!
I don't know how to fix this class of bug completely, but this one instance can simply be fixed by syncing with the `net` thread. The issue is that net_processing still has a stale reference to the disconnected node and is waiting for the `net` thread to delete it. There is no direct way to sync with the Bitcoin Core `net` thread from outside the process, but as a hacky workaround, one can add some bytes to the b
...
(https://github.com/bitcoin/bitcoin/issues/31700#issuecomment-2733704193)
@tnndbtc Thank you for the exact diff to reproduce. This makes it easier to test!
I don't know how to fix this class of bug completely, but this one instance can simply be fixed by syncing with the `net` thread. The issue is that net_processing still has a stale reference to the disconnected node and is waiting for the `net` thread to delete it. There is no direct way to sync with the Bitcoin Core `net` thread from outside the process, but as a hacky workaround, one can add some bytes to the b
...
✅ remyers closed a pull request: "wallet: add coin selection parameter `add_excess_to_recipient_position` for changeless txs with excess that would be added to fees"
(https://github.com/bitcoin/bitcoin/pull/30080)
(https://github.com/bitcoin/bitcoin/pull/30080)
💬 remyers commented on pull request "wallet: add coin selection parameter `add_excess_to_recipient_position` for changeless txs with excess that would be added to fees":
(https://github.com/bitcoin/bitcoin/pull/30080#issuecomment-2733706702)
I now believe we can achieve equivalent behavior without changing the wallet by just using it to find the best funding utxo, and if there is change we can (when appropriate) build the actual tx with the change amount added to the recipient output position before submitting the transaction.
I'll close this PR for now because this doesn't seem generally useful outside of Lightning liquidity providers that want to optimizing their UTXO set management, and a good alternative way of doing it exist
...
(https://github.com/bitcoin/bitcoin/pull/30080#issuecomment-2733706702)
I now believe we can achieve equivalent behavior without changing the wallet by just using it to find the best funding utxo, and if there is change we can (when appropriate) build the actual tx with the change amount added to the recipient output position before submitting the transaction.
I'll close this PR for now because this doesn't seem generally useful outside of Lightning liquidity providers that want to optimizing their UTXO set management, and a good alternative way of doing it exist
...
👍 theStack approved a pull request: "test: replace assert with assert_equal and assert_greater_than"
(https://github.com/bitcoin/bitcoin/pull/32091#pullrequestreview-2695106208)
utACK 387385ba1edf9febdc75d39bd77b35b29714b3d0
(https://github.com/bitcoin/bitcoin/pull/32091#pullrequestreview-2695106208)
utACK 387385ba1edf9febdc75d39bd77b35b29714b3d0
💬 theStack commented on pull request "test: replace assert with assert_equal and assert_greater_than":
(https://github.com/bitcoin/bitcoin/pull/32091#discussion_r2001370725)
> I ran the entire test suite from my `build` directory using `ctest --output-on-failure` and the modified test passed just fine there. Any idea on what might cause the discrepancy?
I assume you built bitcoind without USDT support. One simply way to do enable it is e.g. configuring CMake via `cmake --preset dev-mode`. What happens if you run the individual test, i.e. `$ ./build/test/functional/interface_usdt_net.py`?
(https://github.com/bitcoin/bitcoin/pull/32091#discussion_r2001370725)
> I ran the entire test suite from my `build` directory using `ctest --output-on-failure` and the modified test passed just fine there. Any idea on what might cause the discrepancy?
I assume you built bitcoind without USDT support. One simply way to do enable it is e.g. configuring CMake via `cmake --preset dev-mode`. What happens if you run the individual test, i.e. `$ ./build/test/functional/interface_usdt_net.py`?
💬 maflcko commented on pull request "test: replace assert with assert_equal and assert_greater_than":
(https://github.com/bitcoin/bitcoin/pull/32091#issuecomment-2733763095)
lgtm ACK 387385ba1edf9febdc75d39bd77b35b29714b3d0
(https://github.com/bitcoin/bitcoin/pull/32091#issuecomment-2733763095)
lgtm ACK 387385ba1edf9febdc75d39bd77b35b29714b3d0
💬 maflcko commented on pull request "multiprocess: Add bitcoin-gui -ipcconnect option":
(https://github.com/bitcoin/bitcoin/pull/19461#issuecomment-2733810899)
(The arm CI failed)
(https://github.com/bitcoin/bitcoin/pull/19461#issuecomment-2733810899)
(The arm CI failed)
🤔 ryanofsky reviewed a pull request: "multiprocess: Add libmultiprocess git subtree"
(https://github.com/bitcoin/bitcoin/pull/31741#pullrequestreview-2695196075)
re: https://github.com/bitcoin/bitcoin/pull/31741#pullrequestreview-2694724554
> lgtm, but would you like people to review #31992 first?
Could be helpful but we have a chain of PR's here #31992 #31741 #30975 #31802 that could be reviewed in any order even if they need to be merged in a particular order. Different reviewers will be familiar with different parts of the changes, so it is probably most helpful for reviewers to give feedback on whatever parts they are most familiar with.
(https://github.com/bitcoin/bitcoin/pull/31741#pullrequestreview-2695196075)
re: https://github.com/bitcoin/bitcoin/pull/31741#pullrequestreview-2694724554
> lgtm, but would you like people to review #31992 first?
Could be helpful but we have a chain of PR's here #31992 #31741 #30975 #31802 that could be reviewed in any order even if they need to be merged in a particular order. Different reviewers will be familiar with different parts of the changes, so it is probably most helpful for reviewers to give feedback on whatever parts they are most familiar with.
💬 ryanofsky commented on pull request "multiprocess: Add libmultiprocess git subtree":
(https://github.com/bitcoin/bitcoin/pull/31741#discussion_r2001400939)
re: https://github.com/bitcoin/bitcoin/pull/31741#discussion_r2001144249
> Maybe add `--numeric-owner` here as suggested in https://reproducible-builds.org/docs/archives/ (and as already done further down in this file).
Previously I used that page as a reference and added options to make these tarballs always reproducible but I had to drop them because they were not supported on macos: https://github.com/chaincodelabs/libmultiprocess/issues/139#issuecomment-2621360345, https://github.com/r
...
(https://github.com/bitcoin/bitcoin/pull/31741#discussion_r2001400939)
re: https://github.com/bitcoin/bitcoin/pull/31741#discussion_r2001144249
> Maybe add `--numeric-owner` here as suggested in https://reproducible-builds.org/docs/archives/ (and as already done further down in this file).
Previously I used that page as a reference and added options to make these tarballs always reproducible but I had to drop them because they were not supported on macos: https://github.com/chaincodelabs/libmultiprocess/issues/139#issuecomment-2621360345, https://github.com/r
...
💬 maflcko commented on issue "intermittent issue in p2p_orphan_handling.py":
(https://github.com/bitcoin/bitcoin/issues/31700#issuecomment-2733834009)
An alternative fix would be to adjust `wait_for_parent_requests` to accept any order and any sequence of messages, but this may be a larger diff.
(https://github.com/bitcoin/bitcoin/issues/31700#issuecomment-2733834009)
An alternative fix would be to adjust `wait_for_parent_requests` to accept any order and any sequence of messages, but this may be a larger diff.
🤔 mzumsande reviewed a pull request: "i2p: make a time gap between creating transient sessions and using them"
(https://github.com/bitcoin/bitcoin/pull/32065#pullrequestreview-2695264617)
Two thoughts:
- As far as I understand it (https://geti2p.net/en/about/performance/future), i2p tunnels last for 10 minutes by default. If we don't use these extra sessions before they expire, their creation would be a waste of ressources. I would image this to be particularly relevant in a scenario where i2p is used together with other networks. When we have no need for new regular outbounds because we are full and none of our peers disconnect, we make feeler connections every 2 minutes and ex
...
(https://github.com/bitcoin/bitcoin/pull/32065#pullrequestreview-2695264617)
Two thoughts:
- As far as I understand it (https://geti2p.net/en/about/performance/future), i2p tunnels last for 10 minutes by default. If we don't use these extra sessions before they expire, their creation would be a waste of ressources. I would image this to be particularly relevant in a scenario where i2p is used together with other networks. When we have no need for new regular outbounds because we are full and none of our peers disconnect, we make feeler connections every 2 minutes and ex
...
📝 maflcko opened a pull request: "test: Fix intermittent issue in p2p_orphan_handling.py"
(https://github.com/bitcoin/bitcoin/pull/32092)
The test may fail intermittently when the `net` thread is lagging while calling `DeleteNode`. This may result in a split `getdata`, meaning that `peer2.wait_for_parent_requests([int(parent_peekaboo_AB["txid"], 16), int(parent_missing["txid"], 16)])` fails.
Fix it by adding a sync on the `net` thread.
Fixes #31700
(https://github.com/bitcoin/bitcoin/pull/32092)
The test may fail intermittently when the `net` thread is lagging while calling `DeleteNode`. This may result in a split `getdata`, meaning that `peer2.wait_for_parent_requests([int(parent_peekaboo_AB["txid"], 16), int(parent_missing["txid"], 16)])` fails.
Fix it by adding a sync on the `net` thread.
Fixes #31700
💬 maflcko commented on pull request "test: Fix intermittent issue in p2p_orphan_handling.py":
(https://github.com/bitcoin/bitcoin/pull/32092#issuecomment-2733932368)
Can be tested by the diff provided by @tnndbtc in https://github.com/bitcoin/bitcoin/pull/32063#issuecomment-2730664318 (thanks!):
```diff
diff --git a/src/net.cpp b/src/net.cpp
index 735985a841..2bb6db0cf7 100644
--- a/src/net.cpp
+++ b/src/net.cpp
@@ -1965,6 +1965,9 @@ void CConnman::DisconnectNodes()
// Destroy the object only after other threads have stopped using it.
if (pnode->GetRefCount() <= 0) {
m_nodes_disconnected.remove(pnode);
+
...
(https://github.com/bitcoin/bitcoin/pull/32092#issuecomment-2733932368)
Can be tested by the diff provided by @tnndbtc in https://github.com/bitcoin/bitcoin/pull/32063#issuecomment-2730664318 (thanks!):
```diff
diff --git a/src/net.cpp b/src/net.cpp
index 735985a841..2bb6db0cf7 100644
--- a/src/net.cpp
+++ b/src/net.cpp
@@ -1965,6 +1965,9 @@ void CConnman::DisconnectNodes()
// Destroy the object only after other threads have stopped using it.
if (pnode->GetRefCount() <= 0) {
m_nodes_disconnected.remove(pnode);
+
...
💬 fjahr commented on pull request "test: switch wallet_crosschain.py to signet and drop testnet4":
(https://github.com/bitcoin/bitcoin/pull/32088#issuecomment-2734008623)
utACK cec14ee47d71d8dd5ca8f90b760d807c3c8933a5
Looks good to me and CI failure seems unrelated.
(https://github.com/bitcoin/bitcoin/pull/32088#issuecomment-2734008623)
utACK cec14ee47d71d8dd5ca8f90b760d807c3c8933a5
Looks good to me and CI failure seems unrelated.
💬 ryanofsky commented on pull request "multiprocess: Add bitcoin wrapper executable":
(https://github.com/bitcoin/bitcoin/pull/31375#issuecomment-2734048299)
Rebased 623c0f0df878968dabbaa271a7654c638603f369 -> a9935d4dae8847d0549ec51ddc4349c7e8c61763 ([`pr/wrap.23`](https://github.com/ryanofsky/bitcoin/commits/pr/wrap.23) -> [`pr/wrap.24`](https://github.com/ryanofsky/bitcoin/commits/pr/wrap.24), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/wrap.23-rebase..pr/wrap.24)) due to conflict with #32019
(https://github.com/bitcoin/bitcoin/pull/31375#issuecomment-2734048299)
Rebased 623c0f0df878968dabbaa271a7654c638603f369 -> a9935d4dae8847d0549ec51ddc4349c7e8c61763 ([`pr/wrap.23`](https://github.com/ryanofsky/bitcoin/commits/pr/wrap.23) -> [`pr/wrap.24`](https://github.com/ryanofsky/bitcoin/commits/pr/wrap.24), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/wrap.23-rebase..pr/wrap.24)) due to conflict with #32019
👍 vasild approved a pull request: "multiprocess: Add bitcoin wrapper executable"
(https://github.com/bitcoin/bitcoin/pull/31375#pullrequestreview-2695588548)
ACK a9935d4dae8847d0549ec51ddc4349c7e8c61763
(https://github.com/bitcoin/bitcoin/pull/31375#pullrequestreview-2695588548)
ACK a9935d4dae8847d0549ec51ddc4349c7e8c61763
💬 maflcko commented on pull request "test: switch wallet_crosschain.py to signet and drop testnet4":
(https://github.com/bitcoin/bitcoin/pull/32088#issuecomment-2734205095)
lgtm ACK cec14ee47d71d8dd5ca8f90b760d807c3c8933a5 🌰
<details><summary>Show signature</summary>
Signature:
```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: lgtm ACK cec14ee47d71d8dd
...
(https://github.com/bitcoin/bitcoin/pull/32088#issuecomment-2734205095)
lgtm ACK cec14ee47d71d8dd5ca8f90b760d807c3c8933a5 🌰
<details><summary>Show signature</summary>
Signature:
```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: lgtm ACK cec14ee47d71d8dd
...
💬 Sjors commented on pull request "OP_CHECKCONTRACTVERIFY":
(https://github.com/bitcoin/bitcoin/pull/32080#discussion_r2001653317)
How much resources can a (mempool) transaction waste by violating this rule in the last input / output after making the interpreter do a bunch of hashing work?
And is that worse than existing taproot script allows? And would it be better with a restriction on the number of combinations, e.g. if inputs can't refer to other inputs? Or should these bounds checks be done earlier for the whole transaction?
(https://github.com/bitcoin/bitcoin/pull/32080#discussion_r2001653317)
How much resources can a (mempool) transaction waste by violating this rule in the last input / output after making the interpreter do a bunch of hashing work?
And is that worse than existing taproot script allows? And would it be better with a restriction on the number of combinations, e.g. if inputs can't refer to other inputs? Or should these bounds checks be done earlier for the whole transaction?