💬 maflcko commented on pull request "doc: note that you can assume C++20.":
(https://github.com/bitcoin/bitcoin/pull/30136#issuecomment-2414179165)
For reference, CMake has this documented as `set(CMAKE_CXX_STANDARD 20)` and also fails if it is violated.
(https://github.com/bitcoin/bitcoin/pull/30136#issuecomment-2414179165)
For reference, CMake has this documented as `set(CMAKE_CXX_STANDARD 20)` and also fails if it is violated.
📝 ryanofsky converted_to_draft a pull request: "wallet: Avoid potentially writing incorrect best block locator"
(https://github.com/bitcoin/bitcoin/pull/29652)
This PR updates the `CWallet::chainStateFlushed` method to write the `m_last_block_processed` locator to the database as the wallet's best block instead of the chain tip locator.
Saving the last block processed locator as the best block is less fragile than saving `chainStateFlushed` locator as best block.
Right now wallet code relies on `blockConnected` notifications being sent before `chainStateFlushed` notifications, and on the `chainStateFlushed` locator pointing to the last connected
...
(https://github.com/bitcoin/bitcoin/pull/29652)
This PR updates the `CWallet::chainStateFlushed` method to write the `m_last_block_processed` locator to the database as the wallet's best block instead of the chain tip locator.
Saving the last block processed locator as the best block is less fragile than saving `chainStateFlushed` locator as best block.
Right now wallet code relies on `blockConnected` notifications being sent before `chainStateFlushed` notifications, and on the `chainStateFlushed` locator pointing to the last connected
...
💬 l0rinc commented on pull request "bench: add support for custom data directory":
(https://github.com/bitcoin/bitcoin/pull/31000#discussion_r1801361386)
I did see that conversation, but I don't think it's the benchmark's responsibility to reveal the internals of a certain group of tests.
(https://github.com/bitcoin/bitcoin/pull/31000#discussion_r1801361386)
I did see that conversation, but I don't think it's the benchmark's responsibility to reveal the internals of a certain group of tests.
✅ achow101 closed a pull request: "Remove redundant `-datacarrier` option"
(https://github.com/bitcoin/bitcoin/pull/29942)
(https://github.com/bitcoin/bitcoin/pull/29942)
💬 achow101 commented on pull request "Remove redundant `-datacarrier` option":
(https://github.com/bitcoin/bitcoin/pull/29942#issuecomment-2414187917)
This PR does not seem to have attracted much attention from reviewers. As such, it does not seem important enough right now to keep it sitting idle in the list of open PRs.
Closing due to lack of interest.
(https://github.com/bitcoin/bitcoin/pull/29942#issuecomment-2414187917)
This PR does not seem to have attracted much attention from reviewers. As such, it does not seem important enough right now to keep it sitting idle in the list of open PRs.
Closing due to lack of interest.
💬 stickies-v commented on pull request "doc: add guidance for RPC to developer notes":
(https://github.com/bitcoin/bitcoin/pull/30142#discussion_r1801362597)
Oh would also add that deprecated behaviour should be documented in the RPC docs, such as e.g. in https://github.com/bitcoin/bitcoin/blob/0ca1d1bf69ca364393e924cf41becfde1b68126c/src/rpc/net.cpp#L663-L664?
(https://github.com/bitcoin/bitcoin/pull/30142#discussion_r1801362597)
Oh would also add that deprecated behaviour should be documented in the RPC docs, such as e.g. in https://github.com/bitcoin/bitcoin/blob/0ca1d1bf69ca364393e924cf41becfde1b68126c/src/rpc/net.cpp#L663-L664?
💬 stickies-v commented on pull request "doc: add guidance for RPC to developer notes":
(https://github.com/bitcoin/bitcoin/pull/30142#discussion_r1801361376)
Perhaps useful to elaborate a bit more on distinguishing between backwards compatible/incompatible behaviour?
```suggestion
- Implement an associated `-deprecatedrpc=` option to retain previous RPC behavior when modifying the structure of RPC interface JSON in a backwards-incompatible manner. This includes, but is not limited to, changes such as: data types changes (e.g. from `{"result":{"warnings":""}}` to `{"result":{"warnings":[]}}`, changing a value from a string to a number, etc.), log
...
(https://github.com/bitcoin/bitcoin/pull/30142#discussion_r1801361376)
Perhaps useful to elaborate a bit more on distinguishing between backwards compatible/incompatible behaviour?
```suggestion
- Implement an associated `-deprecatedrpc=` option to retain previous RPC behavior when modifying the structure of RPC interface JSON in a backwards-incompatible manner. This includes, but is not limited to, changes such as: data types changes (e.g. from `{"result":{"warnings":""}}` to `{"result":{"warnings":[]}}`, changing a value from a string to a number, etc.), log
...
🤔 stickies-v reviewed a pull request: "doc: add guidance for RPC to developer notes"
(https://github.com/bitcoin/bitcoin/pull/30142#pullrequestreview-2369659601)
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/30142#pullrequestreview-2369659601)
Concept ACK
💬 maflcko commented on pull request "CI: Add label to scripted-diffs":
(https://github.com/bitcoin/bitcoin/pull/31089#issuecomment-2414191913)
It could still be "hacked" by adding two scripted diffs, one that "fails" and one that passes and adds the label.
I don't think this problem can be solved in CI. Only review can catch it.
Again, my recommendation would be to fix the bug that the exit code is ignored.
(https://github.com/bitcoin/bitcoin/pull/31089#issuecomment-2414191913)
It could still be "hacked" by adding two scripted diffs, one that "fails" and one that passes and adds the label.
I don't think this problem can be solved in CI. Only review can catch it.
Again, my recommendation would be to fix the bug that the exit code is ignored.
💬 fanquake commented on pull request "CI: Add label to scripted-diffs":
(https://github.com/bitcoin/bitcoin/pull/31089#issuecomment-2414196127)
Agree with the other comments. Concept -0
(https://github.com/bitcoin/bitcoin/pull/31089#issuecomment-2414196127)
Agree with the other comments. Concept -0
💬 ryanofsky commented on pull request "wallet: Avoid potentially writing incorrect best block locator":
(https://github.com/bitcoin/bitcoin/pull/29652#issuecomment-2414202142)
Marking as draft, because this has a silent merge conflict and I don't think I will be able to fix it right away. I still do think this PR is a good idea, but #30221 can be reviewed too if the alternate approach it implements is preferred (updating the wallet every block even it doesn't change).
(https://github.com/bitcoin/bitcoin/pull/29652#issuecomment-2414202142)
Marking as draft, because this has a silent merge conflict and I don't think I will be able to fix it right away. I still do think this PR is a good idea, but #30221 can be reviewed too if the alternate approach it implements is preferred (updating the wallet every block even it doesn't change).
💬 dergoegge commented on pull request "set `DEFAULT_PERMIT_BAREMULTISIG` to false":
(https://github.com/bitcoin/bitcoin/pull/28217#issuecomment-2414204462)
Concept NACK
It looks like a decent amount of P2MS outputs are still being created by users.
If we change the default, we'll therefore likely end up degrading compact block relay, as miners would still be incentivised to mine these transactions while (presumably) most of the network wouldn't change the default.
(https://github.com/bitcoin/bitcoin/pull/28217#issuecomment-2414204462)
Concept NACK
It looks like a decent amount of P2MS outputs are still being created by users.
If we change the default, we'll therefore likely end up degrading compact block relay, as miners would still be incentivised to mine these transactions while (presumably) most of the network wouldn't change the default.
💬 jonatack commented on pull request "doc: add guidance for RPC to developer notes":
(https://github.com/bitcoin/bitcoin/pull/30142#discussion_r1801392877)
> Include detailed instructions on feature deprecation and re-enabling previous behavior in release notes.
Release notes should refer to the RPC help for details instead of substituting for full documentation. Example: "Please refer to the RPC help of `getbalances` for details."
(https://github.com/bitcoin/bitcoin/pull/30142#discussion_r1801392877)
> Include detailed instructions on feature deprecation and re-enabling previous behavior in release notes.
Release notes should refer to the RPC help for details instead of substituting for full documentation. Example: "Please refer to the RPC help of `getbalances` for details."
👍 stickies-v approved a pull request: "optimization: Preallocate addresses in GetAddr based on nNodes"
(https://github.com/bitcoin/bitcoin/pull/29608#pullrequestreview-2369709974)
ACK 66082ca3488e7ad78149e05631dccd09be03c961
I don't think this is a particularly impactful change but it's very small and straightforward to review.
(https://github.com/bitcoin/bitcoin/pull/29608#pullrequestreview-2369709974)
ACK 66082ca3488e7ad78149e05631dccd09be03c961
I don't think this is a particularly impactful change but it's very small and straightforward to review.
✅ achow101 closed a pull request: "add `-limitdummyscriptdatasize` option"
(https://github.com/bitcoin/bitcoin/pull/29520)
(https://github.com/bitcoin/bitcoin/pull/29520)
💬 achow101 commented on pull request "add `-limitdummyscriptdatasize` option":
(https://github.com/bitcoin/bitcoin/pull/29520#issuecomment-2414258657)
Closing this as it has not had any activity in a while, and feedback has not been addressed.
(https://github.com/bitcoin/bitcoin/pull/29520#issuecomment-2414258657)
Closing this as it has not had any activity in a while, and feedback has not been addressed.
✅ l0rinc closed a pull request: "CI: Add label to scripted-diffs"
(https://github.com/bitcoin/bitcoin/pull/31089)
(https://github.com/bitcoin/bitcoin/pull/31089)
💬 l0rinc commented on pull request "CI: Add label to scripted-diffs":
(https://github.com/bitcoin/bitcoin/pull/31089#issuecomment-2414260328)
> It could still be "hacked" by adding two scripted diffs, one that "fails" and one that passes and adds the label.
Valid concern, thanks for the comments, I'll close this in that case.
(https://github.com/bitcoin/bitcoin/pull/31089#issuecomment-2414260328)
> It could still be "hacked" by adding two scripted diffs, one that "fails" and one that passes and adds the label.
Valid concern, thanks for the comments, I'll close this in that case.
💬 jonatack commented on pull request "doc: add guidance for RPC to developer notes":
(https://github.com/bitcoin/bitcoin/pull/30142#discussion_r1801413165)
> Implement an associated `-deprecatedrpc=` option to retain previous RPC behavior when modifying the structure of RPC interface JSON in a backwards-incompatible manner.
Suggest to lead with what would require this, use simpler language, and mention the period:
"If an RPC will be changed in a backward-incompatible way, add an associated `-deprecatedrpc=` option to retain previous RPC behavior during the deprecation period."
> This includes, but is not limited to, ...
(Is this long se
...
(https://github.com/bitcoin/bitcoin/pull/30142#discussion_r1801413165)
> Implement an associated `-deprecatedrpc=` option to retain previous RPC behavior when modifying the structure of RPC interface JSON in a backwards-incompatible manner.
Suggest to lead with what would require this, use simpler language, and mention the period:
"If an RPC will be changed in a backward-incompatible way, add an associated `-deprecatedrpc=` option to retain previous RPC behavior during the deprecation period."
> This includes, but is not limited to, ...
(Is this long se
...
💬 achow101 commented on pull request "coins: Add move operations to Coin and CCoinsCacheEntry":
(https://github.com/bitcoin/bitcoin/pull/30643#issuecomment-2414281260)
cc @davidgumberg @andrewtoth
(https://github.com/bitcoin/bitcoin/pull/30643#issuecomment-2414281260)
cc @davidgumberg @andrewtoth