🤔 hodlinator reviewed a pull request: "Ephemeral Dust"
(https://github.com/bitcoin/bitcoin/pull/30239#pullrequestreview-2414994647)
Code reviewed up until most of aa7c90a91adcf8ad9d6b94d5d17797cfd0eb5228.
Disclaimer: I posses far from full context when it comes to validation code, but at least as far as I can tell we are only modifying policy, not consensus.
PR title should probably have "p2p:" or some [other prefix](https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#creating-the-pull-request).
(https://github.com/bitcoin/bitcoin/pull/30239#pullrequestreview-2414994647)
Code reviewed up until most of aa7c90a91adcf8ad9d6b94d5d17797cfd0eb5228.
Disclaimer: I posses far from full context when it comes to validation code, but at least as far as I can tell we are only modifying policy, not consensus.
PR title should probably have "p2p:" or some [other prefix](https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#creating-the-pull-request).
💬 hodlinator commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1829045123)
nit: Missing word
```suggestion
# Dust is 1 satoshi since create_self_transfer_multi disallows 0
```
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1829045123)
nit: Missing word
```suggestion
# Dust is 1 satoshi since create_self_transfer_multi disallows 0
```
💬 hodlinator commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1829048604)
nit: I like the transaction being called "dusty" as it creates dust but is not itself dust. Dust refers only to outputs, not to transactions. Would prefer `dusty_txid`.
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1829048604)
nit: I like the transaction being called "dusty" as it creates dust but is not itself dust. Dust refers only to outputs, not to transactions. Would prefer `dusty_txid`.
💬 hodlinator commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1831063128)
nit: Since we unconditionally insert at the end of the loop, we might as well `insert` up here instead and check if it already existed in the set.
```suggestion
if (!processed_parent_set.insert(parent_txid).second) continue;
```
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1831063128)
nit: Since we unconditionally insert at the end of the loop, we might as well `insert` up here instead and check if it already existed in the set.
```suggestion
if (!processed_parent_set.insert(parent_txid).second) continue;
```
💬 hodlinator commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1831120099)
There's an edge-case where `expected` accidentally contains duplicate `tx` instances (since it's a list, not a set), meaning they might exist in the mempool, and the lengths may be equal, but the mempool may contain some other txs.
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1831120099)
There's an edge-case where `expected` accidentally contains duplicate `tx` instances (since it's a list, not a set), meaning they might exist in the mempool, and the lengths may be equal, but the mempool may contain some other txs.
💬 hodlinator commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1831104732)
While it is good to limit dust in the UTXO-set, I'm not sure adding this limitation is wise.
If we want mining pools to use as un-patched versions of Bitcoin Core as possible, we probably shouldn't try to limit what they can do with this RPC.
IMO this commit d147d929f5093f71c39cb7efbb0dd3888f6c8114 should be split out to its own PR.
(My argument is weakened by the fact that this function already requires the transaction to have made it into the mempool in the first place).
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1831104732)
While it is good to limit dust in the UTXO-set, I'm not sure adding this limitation is wise.
If we want mining pools to use as un-patched versions of Bitcoin Core as possible, we probably shouldn't try to limit what they can do with this RPC.
IMO this commit d147d929f5093f71c39cb7efbb0dd3888f6c8114 should be split out to its own PR.
(My argument is weakened by the fact that this function already requires the transaction to have made it into the mempool in the first place).
💬 hodlinator commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1831655381)
More precise:
```suggestion
// Enforces 0-fee for transactions with dust outputs, no incentive to be mined alone
```
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1831655381)
More precise:
```suggestion
// Enforces 0-fee for transactions with dust outputs, no incentive to be mined alone
```
💬 hodlinator commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1831720019)
All `test_`-methods except `sponsor_cycle` have implementations in the order they are called here, please reorder the call or where in the file the implementation appears.
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1831720019)
All `test_`-methods except `sponsor_cycle` have implementations in the order they are called here, please reorder the call or where in the file the implementation appears.
💬 hodlinator commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1831776816)
Slightly easier to follow comments?
```suggestion
# Setup valid sweep in mempool
sweep_tx = self.wallet.create_self_transfer_multi(utxos_to_spend=dusty_tx["new_utxos"], version=3)
self.nodes[0].submitpackage([dusty_tx["hex"], sweep_tx["hex"]])
assert_mempool_contents(self, self.nodes[0], expected=[dusty_tx["tx"], sweep_tx["tx"]])
# RBF-attempt fails when not spending in-mempool dust output from parent
unspent_sweep_tx = self.wallet.create_
...
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1831776816)
Slightly easier to follow comments?
```suggestion
# Setup valid sweep in mempool
sweep_tx = self.wallet.create_self_transfer_multi(utxos_to_spend=dusty_tx["new_utxos"], version=3)
self.nodes[0].submitpackage([dusty_tx["hex"], sweep_tx["hex"]])
assert_mempool_contents(self, self.nodes[0], expected=[dusty_tx["tx"], sweep_tx["tx"]])
# RBF-attempt fails when not spending in-mempool dust output from parent
unspent_sweep_tx = self.wallet.create_
...
💬 hodlinator commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1831750845)
nit: Less backwards?
```suggestion
def test_parent_with_fee(self):
```
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1831750845)
nit: Less backwards?
```suggestion
def test_parent_with_fee(self):
```
💬 vasild commented on pull request "fuzz: fix bad alloc in connman target":
(https://github.com/bitcoin/bitcoin/pull/31235#issuecomment-2461406655)
I think that the problem warrants a fix in the real code, not in the test. `AddrManImpl::GetAddr_()` would try to overallocate if the caller passes > 100% which is meaningless. It already handles `max_addresses` properly: `nNodes = std::min(nNodes, max_addresses);`. If there are 1000 addresses and the caller asks for 5000, it would clamp it down to 1000.
<details>
<summary>[patch] clamp down the % of addresses, like already done for `max_addresses`</summary>
```diff
diff --git i/src/addr
...
(https://github.com/bitcoin/bitcoin/pull/31235#issuecomment-2461406655)
I think that the problem warrants a fix in the real code, not in the test. `AddrManImpl::GetAddr_()` would try to overallocate if the caller passes > 100% which is meaningless. It already handles `max_addresses` properly: `nNodes = std::min(nNodes, max_addresses);`. If there are 1000 addresses and the caller asks for 5000, it would clamp it down to 1000.
<details>
<summary>[patch] clamp down the % of addresses, like already done for `max_addresses`</summary>
```diff
diff --git i/src/addr
...
👍 theStack approved a pull request: "TxDownloadManager followups"
(https://github.com/bitcoin/bitcoin/pull/31190#pullrequestreview-2420051892)
ACK 5dc94d13d419b8d5e543cb50edeb872335c090e7
(https://github.com/bitcoin/bitcoin/pull/31190#pullrequestreview-2420051892)
ACK 5dc94d13d419b8d5e543cb50edeb872335c090e7
👍 Joj501 approved a pull request: "TxDownloadManager followups"
(https://github.com/bitcoin/bitcoin/pull/31190#pullrequestreview-2420077863)
Connect with my wallet address
(https://github.com/bitcoin/bitcoin/pull/31190#pullrequestreview-2420077863)
Connect with my wallet address
🤔 theStack reviewed a pull request: "net: Use actual memory size in receive buffer accounting"
(https://github.com/bitcoin/bitcoin/pull/31164#pullrequestreview-2420152018)
post-merge ACK d22a234ed270286b483aec2db1e2f716b9756231
(https://github.com/bitcoin/bitcoin/pull/31164#pullrequestreview-2420152018)
post-merge ACK d22a234ed270286b483aec2db1e2f716b9756231
💬 vasild commented on issue "Remove libevent as a dependency (HTTP / cli / torcontrol)":
(https://github.com/bitcoin/bitcoin/issues/31194#issuecomment-2461527876)
> I have been working on the removal of libevent from torcontrol but I still see some errors that I need to debug. I hope I can open a PR soon.
:heart:
poke me for review
(https://github.com/bitcoin/bitcoin/issues/31194#issuecomment-2461527876)
> I have been working on the removal of libevent from torcontrol but I still see some errors that I need to debug. I hope I can open a PR soon.
:heart:
poke me for review
💬 Reliestaff commented on issue "Remove libevent as a dependency (HTTP / cli / torcontrol)":
(https://github.com/bitcoin/bitcoin/issues/31194#issuecomment-2461637396)
Agree
On Thu, Nov 7, 2024, 2:46 AM Vasil Dimov ***@***.***> wrote:
> I have been working on the removal of libevent from torcontrol but I still
> see some errors that I need to debug. I hope I can open a PR soon.
>
> ❤️
>
> poke me for review
>
> —
> Reply to this email directly, view it on GitHub
> <https://github.com/bitcoin/bitcoin/issues/31194#issuecomment-2461527876>,
> or unsubscribe
> <https://github.com/notifications/unsubscribe-auth/BDSRHFZPVAHZK2F7J7W2T5LZ7MLGLAVCNFSM6
...
(https://github.com/bitcoin/bitcoin/issues/31194#issuecomment-2461637396)
Agree
On Thu, Nov 7, 2024, 2:46 AM Vasil Dimov ***@***.***> wrote:
> I have been working on the removal of libevent from torcontrol but I still
> see some errors that I need to debug. I hope I can open a PR soon.
>
> ❤️
>
> poke me for review
>
> —
> Reply to this email directly, view it on GitHub
> <https://github.com/bitcoin/bitcoin/issues/31194#issuecomment-2461527876>,
> or unsubscribe
> <https://github.com/notifications/unsubscribe-auth/BDSRHFZPVAHZK2F7J7W2T5LZ7MLGLAVCNFSM6
...
🤔 maflcko reviewed a pull request: "Improve parallel script validation error debug logging"
(https://github.com/bitcoin/bitcoin/pull/31112#pullrequestreview-2420377681)
Left a nit. Also, the tests seem to fail when running on each commit individually.
(https://github.com/bitcoin/bitcoin/pull/31112#pullrequestreview-2420377681)
Left a nit. Also, the tests seem to fail when running on each commit individually.
💬 maflcko commented on pull request "Improve parallel script validation error debug logging":
(https://github.com/bitcoin/bitcoin/pull/31112#discussion_r1832329946)
nit in f1fa6a5dda8f6b640b6377651e6d76df7ff77e14: Not sure about removing the early-return on invalid just to have it trickle through the rest of the code to a debug log statement. As seen in your last force-push, this makes the code more verbose, complex and brittle (easy to get wrong). (https://github.com/bitcoin/bitcoin/compare/28d67cd01c546fa9ce0d98be208ab6a19f1efbb0..f1fa6a5dda8f6b640b6377651e6d76df7ff77e14)
What do you think about adding a small logging lambda function, with `LogInfo("Bl
...
(https://github.com/bitcoin/bitcoin/pull/31112#discussion_r1832329946)
nit in f1fa6a5dda8f6b640b6377651e6d76df7ff77e14: Not sure about removing the early-return on invalid just to have it trickle through the rest of the code to a debug log statement. As seen in your last force-push, this makes the code more verbose, complex and brittle (easy to get wrong). (https://github.com/bitcoin/bitcoin/compare/28d67cd01c546fa9ce0d98be208ab6a19f1efbb0..f1fa6a5dda8f6b640b6377651e6d76df7ff77e14)
What do you think about adding a small logging lambda function, with `LogInfo("Bl
...
💬 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-2461799300)
> It's not entirely clear to me why BnB should care about whether the excess is going to the recipient or not. I don't think that should change the algorithm at all, so the changes to `SelectCoinsBnB` seem unnecessary.
Imagine this scenario:
- target is 100
- max_excess is 30
- UTXOs: [20, 20, 20, 20, 20, 120]
- fee to spend an input: 4
- minimum spendable: 10
Input set A: [20,20,20,20,20]
Input set B: [120]
Current BnB computes waste as:
Input set A: 20 (best)
Input set
...
(https://github.com/bitcoin/bitcoin/pull/30080#issuecomment-2461799300)
> It's not entirely clear to me why BnB should care about whether the excess is going to the recipient or not. I don't think that should change the algorithm at all, so the changes to `SelectCoinsBnB` seem unnecessary.
Imagine this scenario:
- target is 100
- max_excess is 30
- UTXOs: [20, 20, 20, 20, 20, 120]
- fee to spend an input: 4
- minimum spendable: 10
Input set A: [20,20,20,20,20]
Input set B: [120]
Current BnB computes waste as:
Input set A: 20 (best)
Input set
...
👍 dergoegge approved a pull request: "fuzz: Limit wallet_notifications iterations"
(https://github.com/bitcoin/bitcoin/pull/31238#pullrequestreview-2420537659)
lgtm ACK fa461d7a43aa5a321278c8f734e80fb7aa79bfdb
(https://github.com/bitcoin/bitcoin/pull/31238#pullrequestreview-2420537659)
lgtm ACK fa461d7a43aa5a321278c8f734e80fb7aa79bfdb
🚀 fanquake merged a pull request: "fuzz: Limit wallet_notifications iterations"
(https://github.com/bitcoin/bitcoin/pull/31238)
(https://github.com/bitcoin/bitcoin/pull/31238)