💬 luke-jr commented on pull request "rpc: add optional peer_ids param to filter getpeerinfo":
(https://github.com/bitcoin/bitcoin/pull/32741#issuecomment-3075411770)
I would do peer_id|peer_ids and accept both a single number (most common use) or an array (efficient).
(https://github.com/bitcoin/bitcoin/pull/32741#issuecomment-3075411770)
I would do peer_id|peer_ids and accept both a single number (most common use) or an array (efficient).
💬 w0xlt commented on pull request "wallet: Remove wallet version and several legacy related functions":
(https://github.com/bitcoin/bitcoin/pull/32977#discussion_r2208578726)
I had changed it to `WriteLatestLegacyWalletVersion` before seeing your comment.
If that doesn't work, we can change it to something else.
(https://github.com/bitcoin/bitcoin/pull/32977#discussion_r2208578726)
I had changed it to `WriteLatestLegacyWalletVersion` before seeing your comment.
If that doesn't work, we can change it to something else.
💬 lifofifoX commented on pull request "Reduce minrelaytxfee to 100 sats/kvB":
(https://github.com/bitcoin/bitcoin/pull/32959#issuecomment-3075441263)
It's ridiculous to obsess over "spam" and have it dictate policy, rather than rely on economic incentives. Dust limits should be set based on it being economical to spend a UTXO at minimum relay fees, rather than your misplaced notion of "spam".
(https://github.com/bitcoin/bitcoin/pull/32959#issuecomment-3075441263)
It's ridiculous to obsess over "spam" and have it dictate policy, rather than rely on economic incentives. Dust limits should be set based on it being economical to spend a UTXO at minimum relay fees, rather than your misplaced notion of "spam".
💬 ishaanam commented on pull request "wallet, rpc: add v3 transaction creation and wallet support":
(https://github.com/bitcoin/bitcoin/pull/32896#discussion_r2208593588)
Done
(https://github.com/bitcoin/bitcoin/pull/32896#discussion_r2208593588)
Done
💬 ishaanam commented on pull request "wallet, rpc: add v3 transaction creation and wallet support":
(https://github.com/bitcoin/bitcoin/pull/32896#discussion_r2208595036)
Done
(https://github.com/bitcoin/bitcoin/pull/32896#discussion_r2208595036)
Done
💬 yuvicc commented on pull request "test: Fix reorg patterns in tests to use proper fork-based approach":
(https://github.com/bitcoin/bitcoin/pull/32587#issuecomment-3075479833)
> Across several test files, `trigger_reorg` is defined twice:
>
I am not sure if you have reviewed the code nicely, there isn't any duplication but rather a different approach using time-based timelock, see [comment](https://github.com/bitcoin/bitcoin/pull/32587#discussion_r2180262495)
>
> Only the node lookup differs. Would it make sense to extract a single trigger_reorg(fork_blocks, node) utility in test_framework/blocktools.py and have all tests call that?
For now, I think we ca
...
(https://github.com/bitcoin/bitcoin/pull/32587#issuecomment-3075479833)
> Across several test files, `trigger_reorg` is defined twice:
>
I am not sure if you have reviewed the code nicely, there isn't any duplication but rather a different approach using time-based timelock, see [comment](https://github.com/bitcoin/bitcoin/pull/32587#discussion_r2180262495)
>
> Only the node lookup differs. Would it make sense to extract a single trigger_reorg(fork_blocks, node) utility in test_framework/blocktools.py and have all tests call that?
For now, I think we ca
...
💬 BitcoinMechanic commented on pull request "Reduce minrelaytxfee to 100 sats/kvB":
(https://github.com/bitcoin/bitcoin/pull/32959#issuecomment-3075509865)
> It's ridiculous to obsess over "spam" and have it dictate policy, rather than rely on economic incentives. Dust limits should be set based on it being economical to spend a UTXO at minimum relay fees, rather than your misplaced notion of "spam".
This is an ideological stance that ignores how Bitcoin has operated for basically its entire existence.
(https://github.com/bitcoin/bitcoin/pull/32959#issuecomment-3075509865)
> It's ridiculous to obsess over "spam" and have it dictate policy, rather than rely on economic incentives. Dust limits should be set based on it being economical to spend a UTXO at minimum relay fees, rather than your misplaced notion of "spam".
This is an ideological stance that ignores how Bitcoin has operated for basically its entire existence.
💬 willcl-ark commented on pull request "doc: add alpine build instructions":
(https://github.com/bitcoin/bitcoin/pull/32559#discussion_r2208614184)
Updated to `samurai` and removed explicit `xz` (seems preferable to let this be pulled in as a dependency, or not, by `cmake`) in 4f502baf8f649e30d9495760b54080c272882213
(https://github.com/bitcoin/bitcoin/pull/32559#discussion_r2208614184)
Updated to `samurai` and removed explicit `xz` (seems preferable to let this be pulled in as a dependency, or not, by `cmake`) in 4f502baf8f649e30d9495760b54080c272882213
🤔 murchandamus reviewed a pull request: "Reduce minrelaytxfee to 100 sats/kvB"
(https://github.com/bitcoin/bitcoin/pull/32959#pullrequestreview-3022079811)
Reducing `DEFAULT_MIN_RELAY_TX_FEE` only makes sense to me in conjunction with reducing `DEFAULT_BLOCK_MIN_TX_FEE`. Otherwise nodes become subject to free relay attacks that could be used to waste bandwidth across the network with transactions that are not even in danger of getting mined in most blocks.
> `DEFAULT_INCREMENTAL_RELAY_FEE` should be decreased along with this as well.
That’s probably a bad idea, as it would increase the surface for bandwidth wasting attacks 10× even on top of what
...
(https://github.com/bitcoin/bitcoin/pull/32959#pullrequestreview-3022079811)
Reducing `DEFAULT_MIN_RELAY_TX_FEE` only makes sense to me in conjunction with reducing `DEFAULT_BLOCK_MIN_TX_FEE`. Otherwise nodes become subject to free relay attacks that could be used to waste bandwidth across the network with transactions that are not even in danger of getting mined in most blocks.
> `DEFAULT_INCREMENTAL_RELAY_FEE` should be decreased along with this as well.
That’s probably a bad idea, as it would increase the surface for bandwidth wasting attacks 10× even on top of what
...
💬 murchandamus commented on pull request "Reduce minrelaytxfee to 100 sats/kvB":
(https://github.com/bitcoin/bitcoin/pull/32959#discussion_r2208520547)
I don’t understand why this line was changed, could you elaborate?
(https://github.com/bitcoin/bitcoin/pull/32959#discussion_r2208520547)
I don’t understand why this line was changed, could you elaborate?
💬 willcl-ark commented on pull request "doc: add alpine build instructions":
(https://github.com/bitcoin/bitcoin/pull/32559#discussion_r2208620081)
I don't have a good way to test this personally, as I'm primarily using alpine in a container. I've therefore updated the comment here to:
```
User-Space, Statically Defined Tracing (USDT) is not supported or tested on Alpine Linux at this time.
```
...which I think is more accurate having done more research into systemtap/dtrace/usdt support on Alpine. This doesn't preclude someone from getting it working/documenting the steps in the future.
(https://github.com/bitcoin/bitcoin/pull/32559#discussion_r2208620081)
I don't have a good way to test this personally, as I'm primarily using alpine in a container. I've therefore updated the comment here to:
```
User-Space, Statically Defined Tracing (USDT) is not supported or tested on Alpine Linux at this time.
```
...which I think is more accurate having done more research into systemtap/dtrace/usdt support on Alpine. This doesn't preclude someone from getting it working/documenting the steps in the future.
💬 enirox001 commented on pull request "test: Fix reorg patterns in tests to use proper fork-based approach":
(https://github.com/bitcoin/bitcoin/pull/32587#issuecomment-3075576230)
Thanks for the clarification! You’re absolutely right - I should have reviewed the implementation details more carefully. I appreciate you pointing out that these are different approaches (including the time-based timelock approach) rather than simple duplication.
Also, you’re correct about my phrasing - “defined twice” was unclear when it’s actually “defined multiple times across files.”
Makes sense to address this in a follow-up PR along with other tests that need reorg corrections.
(https://github.com/bitcoin/bitcoin/pull/32587#issuecomment-3075576230)
Thanks for the clarification! You’re absolutely right - I should have reviewed the implementation details more carefully. I appreciate you pointing out that these are different approaches (including the time-based timelock approach) rather than simple duplication.
Also, you’re correct about my phrasing - “defined twice” was unclear when it’s actually “defined multiple times across files.”
Makes sense to address this in a follow-up PR along with other tests that need reorg corrections.
💬 instagibbs commented on pull request "Reduce minrelaytxfee to 100 sats/kvB":
(https://github.com/bitcoin/bitcoin/pull/32959#issuecomment-3075577605)
If tests aren't breaking, a good place to start would be to write a test that breaks on these changes, then fix them in feature commit. As of now, nothing breaks which is concerning.
On the meta level: Very low feerate transactions have negligible impact on mining profitability in terms of block templates themselves. It's not obvious that the price going up 10x means we get to lower the default by 10x, because it's a more important system now and the stakes are simply higher. The most persua
...
(https://github.com/bitcoin/bitcoin/pull/32959#issuecomment-3075577605)
If tests aren't breaking, a good place to start would be to write a test that breaks on these changes, then fix them in feature commit. As of now, nothing breaks which is concerning.
On the meta level: Very low feerate transactions have negligible impact on mining profitability in terms of block templates themselves. It's not obvious that the price going up 10x means we get to lower the default by 10x, because it's a more important system now and the stakes are simply higher. The most persua
...
💬 BitcoinMechanic commented on pull request "Reduce minrelaytxfee to 100 sats/kvB":
(https://github.com/bitcoin/bitcoin/pull/32959#issuecomment-3075606609)
@instagibbs
>The most persuasive argument for this would be that miners have diverged from defaults
You continue to confuse cause and effect. Miners mine what gets relayed, not the reverse.
Default relay policy for Core essentially controls what ends up in the blockchain and what doesn't. It's pointless to keep pretending otherwise.
(https://github.com/bitcoin/bitcoin/pull/32959#issuecomment-3075606609)
@instagibbs
>The most persuasive argument for this would be that miners have diverged from defaults
You continue to confuse cause and effect. Miners mine what gets relayed, not the reverse.
Default relay policy for Core essentially controls what ends up in the blockchain and what doesn't. It's pointless to keep pretending otherwise.
💬 waketraindev commented on pull request "rpc: add optional peer_ids param to filter getpeerinfo":
(https://github.com/bitcoin/bitcoin/pull/32741#issuecomment-3075611171)
> I would do peer_id|peer_ids and accept both a single number (most common use) or an array (efficient).
I don't think `RPCArg` currently supports a single argument that can be either a num or an array. One option would be to add two separate optional arguments: one for a single ID and another for the list, then merge both into the filter set internally. That would allow `getpeerinfo 7` for convenience and `getpeerinfo -1 '[1,3,4,7]'` for multiple.
(https://github.com/bitcoin/bitcoin/pull/32741#issuecomment-3075611171)
> I would do peer_id|peer_ids and accept both a single number (most common use) or an array (efficient).
I don't think `RPCArg` currently supports a single argument that can be either a num or an array. One option would be to add two separate optional arguments: one for a single ID and another for the list, then merge both into the filter set internally. That would allow `getpeerinfo 7` for convenience and `getpeerinfo -1 '[1,3,4,7]'` for multiple.
💬 Rob1Ham commented on pull request "Reduce minrelaytxfee to 100 sats/kvB":
(https://github.com/bitcoin/bitcoin/pull/32959#issuecomment-3075654253)
> If tests aren't breaking, a good place to start would be to write a test that breaks on these changes, then fix them in feature commit. As of now, nothing breaks which is concerning.
The original commit did break the test `p2p_ibd_txrelay.py`. I then shared a [PR](https://github.com/RobinLinus/bitcoin/pull/1) to robin's repo which was squash commited and force pushed here, so each commit would pass the test. It was done as a squash commit after being advised to do so by @l0rinc
> Ever
...
(https://github.com/bitcoin/bitcoin/pull/32959#issuecomment-3075654253)
> If tests aren't breaking, a good place to start would be to write a test that breaks on these changes, then fix them in feature commit. As of now, nothing breaks which is concerning.
The original commit did break the test `p2p_ibd_txrelay.py`. I then shared a [PR](https://github.com/RobinLinus/bitcoin/pull/1) to robin's repo which was squash commited and force pushed here, so each commit would pass the test. It was done as a squash commit after being advised to do so by @l0rinc
> Ever
...
💬 instagibbs commented on pull request "Reduce minrelaytxfee to 100 sats/kvB":
(https://github.com/bitcoin/bitcoin/pull/32959#issuecomment-3075659918)
@Rob1Ham sorry I meant a more direct test rather than "we have poorly written tests in master" which is kinda often (though maybe that test was direct, wasn't immediately clear from commit message)
(https://github.com/bitcoin/bitcoin/pull/32959#issuecomment-3075659918)
@Rob1Ham sorry I meant a more direct test rather than "we have poorly written tests in master" which is kinda often (though maybe that test was direct, wasn't immediately clear from commit message)
🤔 glozow reviewed a pull request: "Reduce minrelaytxfee to 100 sats/kvB"
(https://github.com/bitcoin/bitcoin/pull/32959#pullrequestreview-3022316517)
I don't think this PR changed the min relay fee (see all `assert_equal(node.getmempoolinfo()["minrelaytxfee"], Decimal('0.00010000'))` tests). A 0.1sat/vB tx is still rejected.
I agree the dust feerate is largely orthogonal, but the other minimum feerates are tied, hence autocorrection in mempool_args:
The incremental relay feerate, at least for RBF rule 4, should be the same as minrelaytxfee, since they represent the "price of bandwidth per vb" for transaction relay. Arguably we should pr
...
(https://github.com/bitcoin/bitcoin/pull/32959#pullrequestreview-3022316517)
I don't think this PR changed the min relay fee (see all `assert_equal(node.getmempoolinfo()["minrelaytxfee"], Decimal('0.00010000'))` tests). A 0.1sat/vB tx is still rejected.
I agree the dust feerate is largely orthogonal, but the other minimum feerates are tied, hence autocorrection in mempool_args:
The incremental relay feerate, at least for RBF rule 4, should be the same as minrelaytxfee, since they represent the "price of bandwidth per vb" for transaction relay. Arguably we should pr
...
💬 l0rinc commented on pull request "[IBD] log start of script validation past `assumevalid` block":
(https://github.com/bitcoin/bitcoin/pull/32975#discussion_r2208715108)
Thanks @maflcko - I couldn't reproduce it with the previous version either (maybe I need more blocks or maybe doesn't reproduce with `-reindex` reliably), but I have change the implementation to be closer to: "if script validation was turned off at the beginning and it just got enabled, log it once".
Let me know what you think.
(https://github.com/bitcoin/bitcoin/pull/32975#discussion_r2208715108)
Thanks @maflcko - I couldn't reproduce it with the previous version either (maybe I need more blocks or maybe doesn't reproduce with `-reindex` reliably), but I have change the implementation to be closer to: "if script validation was turned off at the beginning and it just got enabled, log it once".
Let me know what you think.
💬 murchandamus commented on pull request "Reduce minrelaytxfee to 100 sats/kvB":
(https://github.com/bitcoin/bitcoin/pull/32959#issuecomment-3075737825)
@BitcoinMechanic:
> > The most persuasive argument for this would be that miners have diverged from defaults
>
> You continue to confuse cause and effect. Miners mine what gets relayed, not the reverse.
>
> Default relay policy for Core essentially controls what ends up in the blockchain and what doesn't. It's pointless to keep pretending otherwise.
Neither Bitcoin Core, nor Knots, not even [LibreRelay nodes](https://github.com/petertodd/bitcoin/blob/af41065cb2adb47d1401864fe6b26d96a42
...
(https://github.com/bitcoin/bitcoin/pull/32959#issuecomment-3075737825)
@BitcoinMechanic:
> > The most persuasive argument for this would be that miners have diverged from defaults
>
> You continue to confuse cause and effect. Miners mine what gets relayed, not the reverse.
>
> Default relay policy for Core essentially controls what ends up in the blockchain and what doesn't. It's pointless to keep pretending otherwise.
Neither Bitcoin Core, nor Knots, not even [LibreRelay nodes](https://github.com/petertodd/bitcoin/blob/af41065cb2adb47d1401864fe6b26d96a42
...