💬 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
...
💬 instagibbs commented on pull request "validation: docs and cleanups for MemPoolAccept coins views":
(https://github.com/bitcoin/bitcoin/pull/32973#issuecomment-3075769891)
in op and commit message: `test_mid_package_evaluation` doesn't exist, it's named `test_mid_package_eviction`
The bull case for keeping `test_mid_package_replacement` is that it involves an RBF, not a mempool overflow, right? The comment in that test seems slightly erroneous, mentioning `replaced_tx` needs to be at the bottom of the mempool, when I think it just has to be low feerate enough to be RBF'd?
(https://github.com/bitcoin/bitcoin/pull/32973#issuecomment-3075769891)
in op and commit message: `test_mid_package_evaluation` doesn't exist, it's named `test_mid_package_eviction`
The bull case for keeping `test_mid_package_replacement` is that it involves an RBF, not a mempool overflow, right? The comment in that test seems slightly erroneous, mentioning `replaced_tx` needs to be at the bottom of the mempool, when I think it just has to be low feerate enough to be RBF'd?
💬 Rob1Ham commented on pull request "Reduce minrelaytxfee to 100 sats/kvB":
(https://github.com/bitcoin/bitcoin/pull/32959#discussion_r2208741172)
When I evaluated why the test was failing, I noticed that changing the value for `NORMAL_FEE_FILTER` on its own was insufficient for making the test pass. Digging further into the mechanics on how the policy filters worked, and looking logs on my local version of the code this is what I found:
I made the change from `9170997` to `9936506` because, reducing minrelaytxfee halves min_fee_limit (500→50, [fees.cpp:1090](https://github.com/bitcoin/bitcoin/blob/184159e4f30c6846cd0324ee07b59e7f8121a
...
(https://github.com/bitcoin/bitcoin/pull/32959#discussion_r2208741172)
When I evaluated why the test was failing, I noticed that changing the value for `NORMAL_FEE_FILTER` on its own was insufficient for making the test pass. Digging further into the mechanics on how the policy filters worked, and looking logs on my local version of the code this is what I found:
I made the change from `9170997` to `9936506` because, reducing minrelaytxfee halves min_fee_limit (500→50, [fees.cpp:1090](https://github.com/bitcoin/bitcoin/blob/184159e4f30c6846cd0324ee07b59e7f8121a
...
💬 BitcoinMechanic commented on pull request "Reduce minrelaytxfee to 100 sats/kvB":
(https://github.com/bitcoin/bitcoin/pull/32959#issuecomment-3075830452)
> @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/af41065cb2adb47
...
(https://github.com/bitcoin/bitcoin/pull/32959#issuecomment-3075830452)
> @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/af41065cb2adb47
...
🤔 mzumsande reviewed a pull request: "validation: remove BLOCK_FAILED_CHILD"
(https://github.com/bitcoin/bitcoin/pull/32950#pullrequestreview-3022472981)
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/32950#pullrequestreview-3022472981)
Concept ACK
💬 fanquake commented on pull request "doc: add alpine build instructions":
(https://github.com/bitcoin/bitcoin/pull/32559#discussion_r2208795017)
> seems preferable to let this be pulled in as a dependency, or not, by cmake
Not sure? It's a dep for Qt, and some of its sub deps. Nothing to do with CMake.
(https://github.com/bitcoin/bitcoin/pull/32559#discussion_r2208795017)
> seems preferable to let this be pulled in as a dependency, or not, by cmake
Not sure? It's a dep for Qt, and some of its sub deps. Nothing to do with CMake.
🤔 furszy reviewed a pull request: "doc: add note for watch-only wallet migration"
(https://github.com/bitcoin/bitcoin/pull/32866#pullrequestreview-3022558530)
ACK 9d25880bb720bc675a533098268b9e02f86e17ce
(https://github.com/bitcoin/bitcoin/pull/32866#pullrequestreview-3022558530)
ACK 9d25880bb720bc675a533098268b9e02f86e17ce
🤔 furszy reviewed a pull request: "index: fix wrong assert of current_tip == m_best_block_index"
(https://github.com/bitcoin/bitcoin/pull/32878#pullrequestreview-3022570828)
> General question - should we remove the assert, or would it maybe be better to keep it, but commit the best block index before calling `Rewind()` in `Sync()`?
I'm not sure about this but.. if we go down that road, wouldn't it be simpler to just drop the `current_tip` arg and use `m_best_block_index` internally?
(https://github.com/bitcoin/bitcoin/pull/32878#pullrequestreview-3022570828)
> General question - should we remove the assert, or would it maybe be better to keep it, but commit the best block index before calling `Rewind()` in `Sync()`?
I'm not sure about this but.. if we go down that road, wouldn't it be simpler to just drop the `current_tip` arg and use `m_best_block_index` internally?
💬 waketraindev commented on pull request "rpc: add optional peer_ids param to filter getpeerinfo":
(https://github.com/bitcoin/bitcoin/pull/32741#issuecomment-3076011613)
> I would do peer_id|peer_ids and accept both a single number (most common use) or an array (efficient).
Skipped type checking and added simple cases so `getpeerinfo 7` and `getpeerinfo '[1,2,3]'` should work
(https://github.com/bitcoin/bitcoin/pull/32741#issuecomment-3076011613)
> I would do peer_id|peer_ids and accept both a single number (most common use) or an array (efficient).
Skipped type checking and added simple cases so `getpeerinfo 7` and `getpeerinfo '[1,2,3]'` should work
💬 kurapika007 commented on pull request "Reduce minrelaytxfee to 100 sats/kvB":
(https://github.com/bitcoin/bitcoin/pull/32959#issuecomment-3076029783)
> 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
...
(https://github.com/bitcoin/bitcoin/pull/32959#issuecomment-3076029783)
> 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
...
💬 PJJacobowitz commented on issue "ARM Windows build and release":
(https://github.com/bitcoin/bitcoin/issues/31388#issuecomment-3076030848)
Thank you @maflcko . Would you know how to get this into the mainline and then publish it so Windows on ARM users can download it via the link below?
https://bitcoin.org/en/download
(https://github.com/bitcoin/bitcoin/issues/31388#issuecomment-3076030848)
Thank you @maflcko . Would you know how to get this into the mainline and then publish it so Windows on ARM users can download it via the link below?
https://bitcoin.org/en/download
📝 stickies-v opened a pull request: "rpc: refactor: use string_view in Arg/MaybeArg"
(https://github.com/bitcoin/bitcoin/pull/32983)
The `RPCHelpMan::{Arg,MaybeArg}` helpers avoid copying (potentially) large strings by returning them as `const std::string*` (`MaybeArg`) or `const std::string&` (`Arg`). For `MaybeArg`, this has the not-so-nice effect that users need to deal with raw pointers, potentially also requiring new functions (e.g. [`EnsureUniqueWalletName` ](https://github.com/bitcoin/bitcoin/pull/32845/commits/d127b25199118756122232d9aafff19f1922869b#diff-d8bfcfbdd5fa7d5c52d38c1fe5eeac9ce5c5a794cdfaf683585140fa70a3237
...
(https://github.com/bitcoin/bitcoin/pull/32983)
The `RPCHelpMan::{Arg,MaybeArg}` helpers avoid copying (potentially) large strings by returning them as `const std::string*` (`MaybeArg`) or `const std::string&` (`Arg`). For `MaybeArg`, this has the not-so-nice effect that users need to deal with raw pointers, potentially also requiring new functions (e.g. [`EnsureUniqueWalletName` ](https://github.com/bitcoin/bitcoin/pull/32845/commits/d127b25199118756122232d9aafff19f1922869b#diff-d8bfcfbdd5fa7d5c52d38c1fe5eeac9ce5c5a794cdfaf683585140fa70a3237
...
📝 achow101 opened a pull request: "wallet: Set migrated wallet name only on success"
(https://github.com/bitcoin/bitcoin/pull/32984)
After a wallet is migrated and we are trying to load it, if it could not be loaded, don't try to set the wallet name. Otherwise we have a segfault.
This can be tested by migrated a legacy wallet from another network (e.g. trying to migrate a testnet wallet on mainnet). The fixed behavior is return an error and restore the backup.
I was unable to write a working functional test for this behavior.
(https://github.com/bitcoin/bitcoin/pull/32984)
After a wallet is migrated and we are trying to load it, if it could not be loaded, don't try to set the wallet name. Otherwise we have a segfault.
This can be tested by migrated a legacy wallet from another network (e.g. trying to migrate a testnet wallet on mainnet). The fixed behavior is return an error and restore the backup.
I was unable to write a working functional test for this behavior.