💬 polespinasa commented on pull request "wallet, rpc: deprecate settxfee and paytxfee":
(https://github.com/bitcoin/bitcoin/pull/31278#discussion_r1978092613)
Done :)
(https://github.com/bitcoin/bitcoin/pull/31278#discussion_r1978092613)
Done :)
💬 fjahr commented on pull request "wallet, rpc: deprecate settxfee and paytxfee":
(https://github.com/bitcoin/bitcoin/pull/31278#discussion_r1978079620)
Nit: I would have liked it a bit more if you had added that where `-paytxfee=` is tested directly (above) or here it could use a small comment that this doesn't have anything to do with the check mention in the info log. But since this is temporary it's fine to keep it as is.
(https://github.com/bitcoin/bitcoin/pull/31278#discussion_r1978079620)
Nit: I would have liked it a bit more if you had added that where `-paytxfee=` is tested directly (above) or here it could use a small comment that this doesn't have anything to do with the check mention in the info log. But since this is temporary it's fine to keep it as is.
💬 fjahr commented on pull request "wallet, rpc: deprecate settxfee and paytxfee":
(https://github.com/bitcoin/bitcoin/pull/31278#discussion_r1978103312)
Please add a test for this in `test/functional/rpc_deprecated.py`
(https://github.com/bitcoin/bitcoin/pull/31278#discussion_r1978103312)
Please add a test for this in `test/functional/rpc_deprecated.py`
🤔 fjahr reviewed a pull request: "wallet, rpc: deprecate settxfee and paytxfee"
(https://github.com/bitcoin/bitcoin/pull/31278#pullrequestreview-2655058161)
Looks good to me aside from the missing functional deprecation test.
(https://github.com/bitcoin/bitcoin/pull/31278#pullrequestreview-2655058161)
Looks good to me aside from the missing functional deprecation test.
💬 fjahr commented on pull request "wallet, rpc: deprecate settxfee and paytxfee":
(https://github.com/bitcoin/bitcoin/pull/31278#discussion_r1978061581)
formatting nit (no need to fix unless you repush, can be done in editing of the release notes in the wiki)
``` suggestion
The `-paytxfee` startup option and the `settxfee` RPC are now deprecated and will be removed in Bitcoin Core 31.0. They allowed users to set a static fee rate for wallet transactions, which is discouraged in a dynamic fee environment as it can lead to overpaying or underpaying. Users should instead rely on fee estimation or specify a fee rate per transaction using the fee
...
(https://github.com/bitcoin/bitcoin/pull/31278#discussion_r1978061581)
formatting nit (no need to fix unless you repush, can be done in editing of the release notes in the wiki)
``` suggestion
The `-paytxfee` startup option and the `settxfee` RPC are now deprecated and will be removed in Bitcoin Core 31.0. They allowed users to set a static fee rate for wallet transactions, which is discouraged in a dynamic fee environment as it can lead to overpaying or underpaying. Users should instead rely on fee estimation or specify a fee rate per transaction using the fee
...
💬 willcl-ark commented on pull request "Add assumeutxo chainparams to release-process.md":
(https://github.com/bitcoin/bitcoin/pull/31940#discussion_r1978121178)
addressed in 9dcf4b6c4d183ab069459b9be49ef7acd7d3be4b
(https://github.com/bitcoin/bitcoin/pull/31940#discussion_r1978121178)
addressed in 9dcf4b6c4d183ab069459b9be49ef7acd7d3be4b
💬 willcl-ark commented on pull request "Add assumeutxo chainparams to release-process.md":
(https://github.com/bitcoin/bitcoin/pull/31940#discussion_r1978121463)
changed to "should" in 9dcf4b6c4d183ab069459b9be49ef7acd7d3be4b
(https://github.com/bitcoin/bitcoin/pull/31940#discussion_r1978121463)
changed to "should" in 9dcf4b6c4d183ab069459b9be49ef7acd7d3be4b
💬 fjahr commented on pull request "Add mainnet assumeutxo param at height 880,000":
(https://github.com/bitcoin/bitcoin/pull/31969#issuecomment-2695419909)
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/31969#issuecomment-2695419909)
Concept ACK
💬 fjahr commented on pull request "Drop testnet3":
(https://github.com/bitcoin/bitcoin/pull/31974#issuecomment-2695420685)
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/31974#issuecomment-2695420685)
Concept ACK
💬 jonatack commented on pull request "wallet, rpc: deprecate settxfee and paytxfee":
(https://github.com/bitcoin/bitcoin/pull/31278#discussion_r1978124505)
also s/fee_rate/"fee_rate"/
"discouraged in a dynamic fee environment" -> this leads to the question whether the fee environment is more "dynamic" than before, e.g. when this RPC was introduced, to justify removal. Would be shorter and possibly preferable to write: "They allowed users to set a static fee rate for wallet transactions, which could potentially lead to overpaying or underpaying."
(https://github.com/bitcoin/bitcoin/pull/31278#discussion_r1978124505)
also s/fee_rate/"fee_rate"/
"discouraged in a dynamic fee environment" -> this leads to the question whether the fee environment is more "dynamic" than before, e.g. when this RPC was introduced, to justify removal. Would be shorter and possibly preferable to write: "They allowed users to set a static fee rate for wallet transactions, which could potentially lead to overpaying or underpaying."
💬 willcl-ark commented on pull request "Add assumeutxo chainparams to release-process.md":
(https://github.com/bitcoin/bitcoin/pull/31940#discussion_r1978128755)
Thanks for the check! I actually called `invalidateblock` and then `gettxoutsetinfo` when testing this. I figured `gettxoutsetinfo <hash> <block_height>` was permitted from skimming the RPC helptext, but I didnt' verify!
As this doesn't work, I reverted to my tested method of `invalidateblock` followed by `gettxoutsetinfo` in 9dcf4b6c4d183ab069459b9be49ef7acd7d3be4b (please let me know if there is a better method I'm missing)
(https://github.com/bitcoin/bitcoin/pull/31940#discussion_r1978128755)
Thanks for the check! I actually called `invalidateblock` and then `gettxoutsetinfo` when testing this. I figured `gettxoutsetinfo <hash> <block_height>` was permitted from skimming the RPC helptext, but I didnt' verify!
As this doesn't work, I reverted to my tested method of `invalidateblock` followed by `gettxoutsetinfo` in 9dcf4b6c4d183ab069459b9be49ef7acd7d3be4b (please let me know if there is a better method I'm missing)
💬 fjahr commented on issue "RFC: when to drop testnet3":
(https://github.com/bitcoin/bitcoin/issues/31975#issuecomment-2695434964)
I'm guessing we need to weigh the potential pain for those that have built infrastructure on Testnet3 for a while (wallets with lot's of UTXOs for testing elaborate multisig setups, lightning nodes etc.) with the fact that Testnet3 doesn't seem to be usable for anyone according to some commenters. It think a mailing list post is in order and if nobody complains for good reason (not "I'm rich there") then I would even be fine with removal in v30. We can also do a Github grep and reach out to open
...
(https://github.com/bitcoin/bitcoin/issues/31975#issuecomment-2695434964)
I'm guessing we need to weigh the potential pain for those that have built infrastructure on Testnet3 for a while (wallets with lot's of UTXOs for testing elaborate multisig setups, lightning nodes etc.) with the fact that Testnet3 doesn't seem to be usable for anyone according to some commenters. It think a mailing list post is in order and if nobody complains for good reason (not "I'm rich there") then I would even be fine with removal in v30. We can also do a Github grep and reach out to open
...
💬 laanwj commented on pull request "net: Replace libnatpmp with built-in PCP+NATPMP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#issuecomment-2695437679)
i've checked the 31330 release notes, they're kind of brief but get the gist of the change.
We might want to add a sentence that the NAT-PMP/PCP implementation was replaced. It's not super important to end users but if they encounter problems they'll want to report it.
Also "Setting `-upnp` will now return an error" won't be correct after #31916 is merged.
i'll open a PR for this.
(https://github.com/bitcoin/bitcoin/pull/30043#issuecomment-2695437679)
i've checked the 31330 release notes, they're kind of brief but get the gist of the change.
We might want to add a sentence that the NAT-PMP/PCP implementation was replaced. It's not super important to end users but if they encounter problems they'll want to report it.
Also "Setting `-upnp` will now return an error" won't be correct after #31916 is merged.
i'll open a PR for this.
💬 sipa commented on pull request "cluster mempool: introduce TxGraph":
(https://github.com/bitcoin/bitcoin/pull/31363#issuecomment-2695441852)
@instagibbs So... it seems that the "optimal linearization, truncate, postlinearize, split" does not actually result in minimal chunks (but it does result in feerate-diagram-optimal ones, and connected ones):
```mermaid
graph BT;T18["A: 4"];T19["B: 5"];T20["C: 7"];T20-->T18;T20-->T13;T9["D: 7"];T9-->T18;T9-->T19;T13["E: 6"];T13-->T19;
```
The original (optimal) linearization is [ABEDC]. It's just a single chunk, so really any ordering is equally good.
Truncation removes C. Postlineari
...
(https://github.com/bitcoin/bitcoin/pull/31363#issuecomment-2695441852)
@instagibbs So... it seems that the "optimal linearization, truncate, postlinearize, split" does not actually result in minimal chunks (but it does result in feerate-diagram-optimal ones, and connected ones):
```mermaid
graph BT;T18["A: 4"];T19["B: 5"];T20["C: 7"];T20-->T18;T20-->T13;T9["D: 7"];T9-->T18;T9-->T19;T13["E: 6"];T13-->T19;
```
The original (optimal) linearization is [ABEDC]. It's just a single chunk, so really any ordering is equally good.
Truncation removes C. Postlineari
...
💬 jonatack commented on pull request "cli: return local services in -netinfo":
(https://github.com/bitcoin/bitcoin/pull/31886#issuecomment-2695461472)
Thanks for taking a look!
> Seems fine to add this, though it also takes up a line for information
Right, when calling `-netinfo` without a details level (without the peers list).
I began by adding the services in the versions line, in the same format as in the peers list:
```
Bitcoin Core client v28.99.0 - server 70016/Satoshi:28.99.0/ - services nbwcl2
<-> type net serv v mping ping send recv txn blk hb addrp addrl age asmap id version
in onion nwl2
...
(https://github.com/bitcoin/bitcoin/pull/31886#issuecomment-2695461472)
Thanks for taking a look!
> Seems fine to add this, though it also takes up a line for information
Right, when calling `-netinfo` without a details level (without the peers list).
I began by adding the services in the versions line, in the same format as in the peers list:
```
Bitcoin Core client v28.99.0 - server 70016/Satoshi:28.99.0/ - services nbwcl2
<-> type net serv v mping ping send recv txn blk hb addrp addrl age asmap id version
in onion nwl2
...
💬 glozow commented on pull request "net: Replace libnatpmp with built-in PCP+NATPMP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#issuecomment-2695473173)
Thank you @laanwj! Would you mind adding it to the release notes draft actually, instead of opening a PR? https://github.com/bitcoin-core/bitcoin-devwiki/wiki/29.0-Release-Notes-draft
(https://github.com/bitcoin/bitcoin/pull/30043#issuecomment-2695473173)
Thank you @laanwj! Would you mind adding it to the release notes draft actually, instead of opening a PR? https://github.com/bitcoin-core/bitcoin-devwiki/wiki/29.0-Release-Notes-draft
💬 mzumsande commented on issue "compact blocks in IBD resets m_stalling_since":
(https://github.com/bitcoin/bitcoin/issues/31962#issuecomment-2695499701)
I don't think this would work with blocks below the minchainwork threshold, we should abort [here](https://github.com/bitcoin/bitcoin/blob/3c1f72a36700271c7c1293383549c3be29f28edb/src/net_processing.cpp#L4309) -
so large parts of IBD shouldn't be affected. We will still be in IBD for a while after reaching that threshold though, depending on how up-to-date the bitcoind release is.
Also, the node schedules the blocks it downloads during IBD not as an answer to received headers messages, but on
...
(https://github.com/bitcoin/bitcoin/issues/31962#issuecomment-2695499701)
I don't think this would work with blocks below the minchainwork threshold, we should abort [here](https://github.com/bitcoin/bitcoin/blob/3c1f72a36700271c7c1293383549c3be29f28edb/src/net_processing.cpp#L4309) -
so large parts of IBD shouldn't be affected. We will still be in IBD for a while after reaching that threshold though, depending on how up-to-date the bitcoind release is.
Also, the node schedules the blocks it downloads during IBD not as an answer to received headers messages, but on
...
📝 fjahr opened a pull request: "test: Use rpc_deprecated only for testing deprecation"
(https://github.com/bitcoin/bitcoin/pull/31977)
The comment in `functional/rpc_deprecated.py` says "This test should be used to verify correct behaviour of deprecated RPC methods with and without the -deprecatedrpc flags." I think we can get rid of the "with" part since we can assume that every RPC is already tested in at least one other functional test. (I didn't look but I could verify in our coverage if someone has doubts about that.) In order for this test to continue working, the flag will need to be used there. Otherwise this seems to p
...
(https://github.com/bitcoin/bitcoin/pull/31977)
The comment in `functional/rpc_deprecated.py` says "This test should be used to verify correct behaviour of deprecated RPC methods with and without the -deprecatedrpc flags." I think we can get rid of the "with" part since we can assume that every RPC is already tested in at least one other functional test. (I didn't look but I could verify in our coverage if someone has doubts about that.) In order for this test to continue working, the flag will need to be used there. Otherwise this seems to p
...
💬 fjahr commented on pull request "wallet, rpc: deprecate settxfee and paytxfee":
(https://github.com/bitcoin/bitcoin/pull/31278#discussion_r1978196051)
Note that I think it's enough to test the deprecation error there, see #31977
(https://github.com/bitcoin/bitcoin/pull/31278#discussion_r1978196051)
Note that I think it's enough to test the deprecation error there, see #31977
💬 laanwj commented on pull request "cli: return local services in -netinfo":
(https://github.com/bitcoin/bitcoin/pull/31886#issuecomment-2695543354)
> Nevertheless, I could revert that change if you prefer it remain appended to the versions header, WDYT?
i agree an alphabet soup like "nbwcl2" does not say anything without a legend what every character means.
To be clear, i'm not against adding it, it's only one line, it was more of a general concern about the direction.
(https://github.com/bitcoin/bitcoin/pull/31886#issuecomment-2695543354)
> Nevertheless, I could revert that change if you prefer it remain appended to the versions header, WDYT?
i agree an alphabet soup like "nbwcl2" does not say anything without a legend what every character means.
To be clear, i'm not against adding it, it's only one line, it was more of a general concern about the direction.
📝 glozow opened a pull request: "kernel: pre-29.x chainparams and headerssync update"
(https://github.com/bitcoin/bitcoin/pull/31978)
Update chainparams and headerssync config for v29.0 (see [release process](https://github.com/bitcoin/bitcoin/blob/master/doc/release-process.md#before-branch-off)).
(https://github.com/bitcoin/bitcoin/pull/31978)
Update chainparams and headerssync config for v29.0 (see [release process](https://github.com/bitcoin/bitcoin/blob/master/doc/release-process.md#before-branch-off)).