💬 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
💬 glozow commented on pull request "doc: add dustThreshold explain of P2SH & P2TR":
(https://github.com/bitcoin/bitcoin/pull/30023#discussion_r1801416152)
This specifically does not match the code.
(https://github.com/bitcoin/bitcoin/pull/30023#discussion_r1801416152)
This specifically does not match the code.
💬 jonatack commented on pull request "doc: add guidance for RPC to developer notes":
(https://github.com/bitcoin/bitcoin/pull/30142#discussion_r1801427354)
Yes, we've also traditionally mentioned these in all-caps in the RPC helps, i.e.
```
RPCResult{RPCResult::Type::STR, "warnings", "(DEPRECATED) network and blockchain warnings, if any...)"} :
```
(https://github.com/bitcoin/bitcoin/pull/30142#discussion_r1801427354)
Yes, we've also traditionally mentioned these in all-caps in the RPC helps, i.e.
```
RPCResult{RPCResult::Type::STR, "warnings", "(DEPRECATED) network and blockchain warnings, if any...)"} :
```
💬 achow101 commented on pull request "test: Shut down framework cleanly on RPC connection failure":
(https://github.com/bitcoin/bitcoin/pull/30660#issuecomment-2414327186)
cc @tdb3
(https://github.com/bitcoin/bitcoin/pull/30660#issuecomment-2414327186)
cc @tdb3
💬 manjiechen commented on pull request "doc: add dustThreshold explain of P2SH & P2TR":
(https://github.com/bitcoin/bitcoin/pull/30023#discussion_r1801440711)
> This specifically does not match the code.
Here we want to explain the dust calculation logic of existing common scripts
The code below simplifies the logic of dust level and says in the comment that the dust limit will not be lowered for segwit v1
I think we need to explain the situation clearly, although we have some considerations in engineering to choose a general solution
(https://github.com/bitcoin/bitcoin/pull/30023#discussion_r1801440711)
> This specifically does not match the code.
Here we want to explain the dust calculation logic of existing common scripts
The code below simplifies the logic of dust level and says in the comment that the dust limit will not be lowered for segwit v1
I think we need to explain the situation clearly, although we have some considerations in engineering to choose a general solution