💬 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)
💬 brunoerg commented on pull request "fuzz: fix bad alloc in connman target":
(https://github.com/bitcoin/bitcoin/pull/31235#issuecomment-2461912635)
> I think that the problem warrants a fix in the real code, not in the test.
We can still limit the values to reflect the reality. I don't see how to have a huge `max_pct` in practice. For example, for `GETADDR`, we use `MAX_ADDR_TO_SEND`/`MAX_PCT_ADDR_TO_SEND`. For the RPC `getnodeaddresses` and other usage, its value is always zero.
(https://github.com/bitcoin/bitcoin/pull/31235#issuecomment-2461912635)
> I think that the problem warrants a fix in the real code, not in the test.
We can still limit the values to reflect the reality. I don't see how to have a huge `max_pct` in practice. For example, for `GETADDR`, we use `MAX_ADDR_TO_SEND`/`MAX_PCT_ADDR_TO_SEND`. For the RPC `getnodeaddresses` and other usage, its value is always zero.
💬 maflcko commented on pull request "ci: Split out native fuzz jobs for macOS and windows (take 2)":
(https://github.com/bitcoin/bitcoin/pull/31221#discussion_r1832478373)
It seems a bit annoying that the Windows build is now duplicated and seems to take 2h each times 2 = 4 CI hours total.
I wonder if this could be avoided (or reduced) by using a cmake multi config build. Putting the fuzz result into a separate folder and then using that in the fuzz_runner should be enough?
(https://github.com/bitcoin/bitcoin/pull/31221#discussion_r1832478373)
It seems a bit annoying that the Windows build is now duplicated and seems to take 2h each times 2 = 4 CI hours total.
I wonder if this could be avoided (or reduced) by using a cmake multi config build. Putting the fuzz result into a separate folder and then using that in the fuzz_runner should be enough?
🤔 rkrux reviewed a pull request: "util: Narrow scope of args (-color, -version, -conf, -datadir)"
(https://github.com/bitcoin/bitcoin/pull/31212#pullrequestreview-2420533755)
Concept ACK 9e130811a3f68aea77ddeecf667aa8389a2940e8
Thanks for catching issues here, gave me an opportunity to read the app init code a bit more!
Successful make and functional tests. Left couple suggestions in the first 2 commits. Disallowing `noconf` and `nodatadir` conceptually makes sense to me. I tested with these 2 args and the output I received seems okay to me.
```
➜ bitcoin git:(2024/11/invalid_args) ✗ bitcoinclireg -noconf
Error parsing command line arguments: Negating of
...
(https://github.com/bitcoin/bitcoin/pull/31212#pullrequestreview-2420533755)
Concept ACK 9e130811a3f68aea77ddeecf667aa8389a2940e8
Thanks for catching issues here, gave me an opportunity to read the app init code a bit more!
Successful make and functional tests. Left couple suggestions in the first 2 commits. Disallowing `noconf` and `nodatadir` conceptually makes sense to me. I tested with these 2 args and the output I received seems okay to me.
```
➜ bitcoin git:(2024/11/invalid_args) ✗ bitcoinclireg -noconf
Error parsing command line arguments: Negating of
...