💬 earonesty commented on pull request "policy: uncap datacarrier by default":
(https://github.com/bitcoin/bitcoin/pull/32406#issuecomment-2906258968)
wouldn't it be reasonable to make the cap a "little bigger by default" as
a conservative change, and just to see if it causes an uptick in average
op_return sizes and uses as storage?
its not so urgent to allow 10kb op_returns
and data *carrier* limits are more about protecting the p2p and mempool
layers - not protecting from inclusion in blocks
the knock-on effects of arbitrary data at the p2p layer could be severe in
ways that have nothing to do with block storage
On Fri, May
...
(https://github.com/bitcoin/bitcoin/pull/32406#issuecomment-2906258968)
wouldn't it be reasonable to make the cap a "little bigger by default" as
a conservative change, and just to see if it causes an uptick in average
op_return sizes and uses as storage?
its not so urgent to allow 10kb op_returns
and data *carrier* limits are more about protecting the p2p and mempool
layers - not protecting from inclusion in blocks
the knock-on effects of arbitrary data at the p2p layer could be severe in
ways that have nothing to do with block storage
On Fri, May
...
👍 rkrux approved a pull request: "walletdb: Log additional exception error messages for corrupted wallets"
(https://github.com/bitcoin/bitcoin/pull/32598#pullrequestreview-2866176097)
ACK ad9a13fc424e9deb262e2b1d54bcdc7370263ea0
(https://github.com/bitcoin/bitcoin/pull/32598#pullrequestreview-2866176097)
ACK ad9a13fc424e9deb262e2b1d54bcdc7370263ea0
📝 koyaness opened a pull request: "Small improvement to the documentation"
(https://github.com/bitcoin/bitcoin/pull/32609)
Small improvement to the documentation to improve readability and understanding.
(https://github.com/bitcoin/bitcoin/pull/32609)
Small improvement to the documentation to improve readability and understanding.
💬 vasild commented on pull request "net: replace manual reference counting of CNode with shared_ptr":
(https://github.com/bitcoin/bitcoin/pull/32015#issuecomment-2906544041)
> `CNode` was supposed to become internal and not passed into `PeerManager` at all
Do you need any help with that?
(https://github.com/bitcoin/bitcoin/pull/32015#issuecomment-2906544041)
> `CNode` was supposed to become internal and not passed into `PeerManager` at all
Do you need any help with that?
💬 rkrux commented on pull request "wallet, rpc: clarify wallet version in getwalletinfo help":
(https://github.com/bitcoin/bitcoin/pull/32603#discussion_r2105749398)
Makes sense, done.
(https://github.com/bitcoin/bitcoin/pull/32603#discussion_r2105749398)
Makes sense, done.
💬 spikeyrock commented on issue "avoid using invalidateblock to directly test reorg behavior":
(https://github.com/bitcoin/bitcoin/issues/32531#issuecomment-2906624037)
maybe spin up two regtest nodes, mine a fork on one, and then connect them at just the right time to trigger the reorg. That way, the reorg happens "naturally" through P2P sync rather than forcing it.
(https://github.com/bitcoin/bitcoin/issues/32531#issuecomment-2906624037)
maybe spin up two regtest nodes, mine a fork on one, and then connect them at just the right time to trigger the reorg. That way, the reorg happens "naturally" through P2P sync rather than forcing it.
🤔 rkrux reviewed a pull request: "rpc: Note in fundrawtransaction doc, fee rate is for package"
(https://github.com/bitcoin/bitcoin/pull/32607#pullrequestreview-2866228840)
ACK fe0432b1c4a10b74844c2dedefccfe340c0d3b10
This is a good note, helpful. I verified in the code that the outputs selected in the transaction have a corresponding `ancestor_bump_fees` param that is used in calculating the `TotalBumpFees` inside `CreateTransactionInternal` function.
https://github.com/bitcoin/bitcoin/blob/638a4c0bd8b53766faeb437244b2aae4eed28dcf/src/wallet/coinselection.h#L75-L76
https://github.com/bitcoin/bitcoin/blob/638a4c0bd8b53766faeb437244b2aae4eed28dcf/src/wallet
...
(https://github.com/bitcoin/bitcoin/pull/32607#pullrequestreview-2866228840)
ACK fe0432b1c4a10b74844c2dedefccfe340c0d3b10
This is a good note, helpful. I verified in the code that the outputs selected in the transaction have a corresponding `ancestor_bump_fees` param that is used in calculating the `TotalBumpFees` inside `CreateTransactionInternal` function.
https://github.com/bitcoin/bitcoin/blob/638a4c0bd8b53766faeb437244b2aae4eed28dcf/src/wallet/coinselection.h#L75-L76
https://github.com/bitcoin/bitcoin/blob/638a4c0bd8b53766faeb437244b2aae4eed28dcf/src/wallet
...
💬 rkrux commented on pull request "rpc: Note in fundrawtransaction doc, fee rate is for package":
(https://github.com/bitcoin/bitcoin/pull/32607#discussion_r2105759450)
I see it needs a full stop after `watch-only`.
(https://github.com/bitcoin/bitcoin/pull/32607#discussion_r2105759450)
I see it needs a full stop after `watch-only`.
💬 rkrux commented on pull request "rpc: Note in fundrawtransaction doc, fee rate is for package":
(https://github.com/bitcoin/bitcoin/pull/32607#discussion_r2105759505)
Nit:
```diff
- not the resulting transaction.",
+ not only the resulting transaction.",
```
(https://github.com/bitcoin/bitcoin/pull/32607#discussion_r2105759505)
Nit:
```diff
- not the resulting transaction.",
+ not only the resulting transaction.",
```
💬 zaidmstrr commented on pull request "multiprocess: Add capnp wrapper for Chain interface":
(https://github.com/bitcoin/bitcoin/pull/29409#discussion_r2105780723)
This new PR https://github.com/bitcoin/bitcoin/pull/32438/commits/fa62a013a558338dc6ee5fb4cfd6fc7c782c301b removes this `flush()` from the codebase. I think it should also be removed from `chain.capnp` otherwise it will give build errors.
(https://github.com/bitcoin/bitcoin/pull/29409#discussion_r2105780723)
This new PR https://github.com/bitcoin/bitcoin/pull/32438/commits/fa62a013a558338dc6ee5fb4cfd6fc7c782c301b removes this `flush()` from the codebase. I think it should also be removed from `chain.capnp` otherwise it will give build errors.
🤔 naiyoma reviewed a pull request: "rpc: generatetomany"
(https://github.com/bitcoin/bitcoin/pull/32468#pullrequestreview-2866255285)
I'm a bit confused about the current approach. Are you planning to add `generatetomany` and also extend `generateblock` to accommodate an array of addresses?
I suggest that you put one approach in your fork and indicate this clearly in your description—separation of concerns makes it easier to follow and review.
For example:
Approach 1
Approach 2 → Link to the PR in your fork
(https://github.com/bitcoin/bitcoin/pull/32468#pullrequestreview-2866255285)
I'm a bit confused about the current approach. Are you planning to add `generatetomany` and also extend `generateblock` to accommodate an array of addresses?
I suggest that you put one approach in your fork and indicate this clearly in your description—separation of concerns makes it easier to follow and review.
For example:
Approach 1
Approach 2 → Link to the PR in your fork
💬 polespinasa commented on pull request "rpc: generatetomany":
(https://github.com/bitcoin/bitcoin/pull/32468#issuecomment-2906764458)
> I'm a bit confused about the current approach. Are you planning to add `generatetomany` and also extend `generateblock` to accommodate an array of addresses?
`generatetomany` will not be added. I have to delete that code.
(https://github.com/bitcoin/bitcoin/pull/32468#issuecomment-2906764458)
> I'm a bit confused about the current approach. Are you planning to add `generatetomany` and also extend `generateblock` to accommodate an array of addresses?
`generatetomany` will not be added. I have to delete that code.
💬 maflcko commented on pull request "rpc: generatetomany":
(https://github.com/bitcoin/bitcoin/pull/32468#issuecomment-2906765296)
> `generatetomany` will not be added. I have to delete that code.
Generally, it is best to just push the final version of the code, so that it is ready for further review. Prior versions of the code can trivially be retrieved via the commit hash (or a named alias), if needed.
(https://github.com/bitcoin/bitcoin/pull/32468#issuecomment-2906765296)
> `generatetomany` will not be added. I have to delete that code.
Generally, it is best to just push the final version of the code, so that it is ready for further review. Prior versions of the code can trivially be retrieved via the commit hash (or a named alias), if needed.
💬 naiyoma commented on pull request "rpc: generatetomany":
(https://github.com/bitcoin/bitcoin/pull/32468#discussion_r2105783773)
11 shouldn't be str
```diff
--- a/src/rpc/mining.cpp
+++ b/src/rpc/mining.cpp
@@ -351,7 +351,7 @@ static RPCHelpMan generatetomany()
}},
RPCExamples{
"\nGenerate 11 blocks to two different addresses:\n"
- + HelpExampleCli("generatetomany", "\"11 '[\\\"bcrt1qal6p633hvwz2yp5mav0qy7u2az8gkn2xywnj6v\\\", \\\"bcrt1qvr3qgyhw6y0e0zj97v0j5yc40xtpea4wqj0g43\\\"]'\"")
+ + HelpExampleCli("generatetomany", "11 '[\"bcrt1qal6p633hvwz2yp5mav0
...
(https://github.com/bitcoin/bitcoin/pull/32468#discussion_r2105783773)
11 shouldn't be str
```diff
--- a/src/rpc/mining.cpp
+++ b/src/rpc/mining.cpp
@@ -351,7 +351,7 @@ static RPCHelpMan generatetomany()
}},
RPCExamples{
"\nGenerate 11 blocks to two different addresses:\n"
- + HelpExampleCli("generatetomany", "\"11 '[\\\"bcrt1qal6p633hvwz2yp5mav0qy7u2az8gkn2xywnj6v\\\", \\\"bcrt1qvr3qgyhw6y0e0zj97v0j5yc40xtpea4wqj0g43\\\"]'\"")
+ + HelpExampleCli("generatetomany", "11 '[\"bcrt1qal6p633hvwz2yp5mav0
...
💬 maflcko commented on issue "test: `feature_fee_estimation` failure after duplicate coinbase tx weight reservation fix":
(https://github.com/bitcoin/bitcoin/issues/32461#issuecomment-2906769514)
https://cirrus-ci.com/task/5716739608018944?logs=ci#L3079
(https://github.com/bitcoin/bitcoin/issues/32461#issuecomment-2906769514)
https://cirrus-ci.com/task/5716739608018944?logs=ci#L3079
💬 maflcko commented on pull request "rpc: Note in fundrawtransaction doc, fee rate is for package":
(https://github.com/bitcoin/bitcoin/pull/32607#issuecomment-2906770023)
> Interesting that the `feature_fee_estimation.py ` test fails in the CI coincidentally but seems unrelated to me as it passed in my system. Maybe a rerun/push would fix it?
See https://github.com/bitcoin/bitcoin/issues/32461
(https://github.com/bitcoin/bitcoin/pull/32607#issuecomment-2906770023)
> Interesting that the `feature_fee_estimation.py ` test fails in the CI coincidentally but seems unrelated to me as it passed in my system. Maybe a rerun/push would fix it?
See https://github.com/bitcoin/bitcoin/issues/32461
💬 maflcko commented on pull request "rpc, doc: clarify wallet version in getwalletinfo help":
(https://github.com/bitcoin/bitcoin/pull/32603#discussion_r2105786933)
I don't think this is right. The current wallet version is `169900` (for a freshly created descriptor wallet). However, I don't think a Bitcoin Core 16.99 client can open descriptor wallets at all.
Generally, all modules in Bitcoin Core moved away from the client version to use a module-specific and module-internal version. (You can see this when you look at all other files written (fee estimates, mempool, addr ...). Also the p2p version, but that again has been replaced by feature-messages.)
...
(https://github.com/bitcoin/bitcoin/pull/32603#discussion_r2105786933)
I don't think this is right. The current wallet version is `169900` (for a freshly created descriptor wallet). However, I don't think a Bitcoin Core 16.99 client can open descriptor wallets at all.
Generally, all modules in Bitcoin Core moved away from the client version to use a module-specific and module-internal version. (You can see this when you look at all other files written (fee estimates, mempool, addr ...). Also the p2p version, but that again has been replaced by feature-messages.)
...
💬 maflcko commented on pull request "rpc: Round verificationprogress to 1 for a recent tip":
(https://github.com/bitcoin/bitcoin/pull/32528#discussion_r2105795801)
thx, done because it is less code and the suggestion was fine
(https://github.com/bitcoin/bitcoin/pull/32528#discussion_r2105795801)
thx, done because it is less code and the suggestion was fine
💬 maflcko commented on pull request "rpc: Round verificationprogress to 1 for a recent tip":
(https://github.com/bitcoin/bitcoin/pull/32528#issuecomment-2906792097)
> Might warrant a release note: "Verification progress will be 100% if all known blocks are verified and timestamped within the last two hours when previously a stale tip might result in progress < 100%".
I consider the changes here only stylistic (similar to changing the `bitcoin-cli -color=...` colours). No one should be relying on the informational-only value anyway.
(https://github.com/bitcoin/bitcoin/pull/32528#issuecomment-2906792097)
> Might warrant a release note: "Verification progress will be 100% if all known blocks are verified and timestamped within the last two hours when previously a stale tip might result in progress < 100%".
I consider the changes here only stylistic (similar to changing the `bitcoin-cli -color=...` colours). No one should be relying on the informational-only value anyway.
💬 maflcko commented on pull request "validation: Do less work in NeedsRedownload":
(https://github.com/bitcoin/bitcoin/pull/31714#issuecomment-2906794331)
Yeah, if we are fine with the rare edge case, it should also be fine to fully remove it, and instead replace the "unsupported utxo db" error message from reindex-chainstate to reindex. Alternatively, it could automatically force a reindex to avoid the user picking the wrong setting.
(https://github.com/bitcoin/bitcoin/pull/31714#issuecomment-2906794331)
Yeah, if we are fine with the rare edge case, it should also be fine to fully remove it, and instead replace the "unsupported utxo db" error message from reindex-chainstate to reindex. Alternatively, it could automatically force a reindex to avoid the user picking the wrong setting.