👍 ryanofsky approved a pull request: "refactor: CLI cleanups"
(https://github.com/bitcoin/bitcoin/pull/31887#pullrequestreview-2650921706)
Code review ACK ccc34a26170ef8c63a6f211eda4464e954873e89. Since last review just added a commit replacing IsArgSet call. I left some suggestions that could be followed up, but they are not very important.
(https://github.com/bitcoin/bitcoin/pull/31887#pullrequestreview-2650921706)
Code review ACK ccc34a26170ef8c63a6f211eda4464e954873e89. Since last review just added a commit replacing IsArgSet call. I left some suggestions that could be followed up, but they are not very important.
💬 ryanofsky commented on pull request "refactor: CLI cleanups":
(https://github.com/bitcoin/bitcoin/pull/31887#discussion_r1975653432)
re: https://github.com/bitcoin/bitcoin/pull/31887#discussion_r1966627276
> If you insist on keeping structs, maybe hoist up the braces to conform with the dev-notes?
Not important but do agree it would be good to use recommended style for structs with opening brace on the same line (can just run `clang-format`).
Also agree classes make more sense than structs here. Compilers may not care about the difference but they mean different things to developers. Structs usually provide data defi
...
(https://github.com/bitcoin/bitcoin/pull/31887#discussion_r1975653432)
re: https://github.com/bitcoin/bitcoin/pull/31887#discussion_r1966627276
> If you insist on keeping structs, maybe hoist up the braces to conform with the dev-notes?
Not important but do agree it would be good to use recommended style for structs with opening brace on the same line (can just run `clang-format`).
Also agree classes make more sense than structs here. Compilers may not care about the difference but they mean different things to developers. Structs usually provide data defi
...
💬 ryanofsky commented on pull request "refactor: CLI cleanups":
(https://github.com/bitcoin/bitcoin/pull/31887#discussion_r1975511515)
In commit "cli, refactor: for -getinfo, replace IsArgSet() with GetBoolArg()" (ccc34a26170ef8c63a6f211eda4464e954873e89)
Would be good to drop "refactor" from commit subject since this is a small change in behavior, not a pure refactoring. It be described as a bugfix: no longer treating `-nogetinfo` and `-getinfo=0` the same as `-getinfo` and `-getinfo=1`, instead treating them like getinfo was not specified.
(https://github.com/bitcoin/bitcoin/pull/31887#discussion_r1975511515)
In commit "cli, refactor: for -getinfo, replace IsArgSet() with GetBoolArg()" (ccc34a26170ef8c63a6f211eda4464e954873e89)
Would be good to drop "refactor" from commit subject since this is a small change in behavior, not a pure refactoring. It be described as a bugfix: no longer treating `-nogetinfo` and `-getinfo=0` the same as `-getinfo` and `-getinfo=1`, instead treating them like getinfo was not specified.
📝 GarmashAlex opened a pull request: "tests: Improve stderr validation in test_runner.py"
(https://github.com/bitcoin/bitcoin/pull/31966)
This PR implements the improvement suggested in a TODO comment in test/util/test_runner.py.
It adds validation that stderr is empty when no errors are expected in test cases.
The change adds a check that verifies stderr is empty when no error_txt is specified in
the test object, with a special exception for bitcoin-tx running under Wine, which was
mentioned in the original TODO comment.
This improvement helps catch unexpected error messages that might otherwise go unnoticed
during
...
(https://github.com/bitcoin/bitcoin/pull/31966)
This PR implements the improvement suggested in a TODO comment in test/util/test_runner.py.
It adds validation that stderr is empty when no errors are expected in test cases.
The change adds a check that verifies stderr is empty when no error_txt is specified in
the test object, with a special exception for bitcoin-tx running under Wine, which was
mentioned in the original TODO comment.
This improvement helps catch unexpected error messages that might otherwise go unnoticed
during
...
💬 fanquake commented on pull request "multiprocess: Add libmultiprocess git subtree":
(https://github.com/bitcoin/bitcoin/pull/31741#issuecomment-2691608578)
Before we do this, can we also move the libmultiprocess repository into the bitcoin-core org.
(https://github.com/bitcoin/bitcoin/pull/31741#issuecomment-2691608578)
Before we do this, can we also move the libmultiprocess repository into the bitcoin-core org.
💬 edilmedeiros commented on pull request "contrib/signet/miner: increase miner search space":
(https://github.com/bitcoin/bitcoin/pull/30130#issuecomment-2691907004)
Forgot about this PR. Closing for now, even tough the issue remains.
(https://github.com/bitcoin/bitcoin/pull/30130#issuecomment-2691907004)
Forgot about this PR. Closing for now, even tough the issue remains.
✅ edilmedeiros closed a pull request: "contrib/signet/miner: increase miner search space"
(https://github.com/bitcoin/bitcoin/pull/30130)
(https://github.com/bitcoin/bitcoin/pull/30130)
💬 dongwook-chan commented on pull request "rpc: Support v3 raw transactions creation":
(https://github.com/bitcoin/bitcoin/pull/31936#discussion_r1976310891)
@glozow
Thanks for the feedback. `int64_t` was my bad. However `version` field of raw transaction seems to be `int32_t` type according to [bitcoint developer doc.](https://developer.bitcoin.org/reference/transactions.html#raw-transaction-format). The doc explicitly states that the field is signed. Should I used unsigned instead?
(https://github.com/bitcoin/bitcoin/pull/31936#discussion_r1976310891)
@glozow
Thanks for the feedback. `int64_t` was my bad. However `version` field of raw transaction seems to be `int32_t` type according to [bitcoint developer doc.](https://developer.bitcoin.org/reference/transactions.html#raw-transaction-format). The doc explicitly states that the field is signed. Should I used unsigned instead?
🤔 stratospher reviewed a pull request: "validation: Add eligible ancestors of reconsidered block to setBlockIndexCandidates"
(https://github.com/bitcoin/bitcoin/pull/30479#pullrequestreview-2639253918)
tested ACK 4a76a8300f295e8eab0ea8ab1ea1580450f131c8.
makes sense that `setBlockIndexCandidates` should ideally contain all block indices with as much/more work than current tip regardless of whether it is an ancestor/descendant.
I've tested it by demonstrating a situation where the chain tip doesn't get correctly updated on master - https://github.com/stratospher/bitcoin/commit/57959f9b659f9df13aca3d08b2a14adc9588ac07, feel free to cherry-pick if useful.
(https://github.com/bitcoin/bitcoin/pull/30479#pullrequestreview-2639253918)
tested ACK 4a76a8300f295e8eab0ea8ab1ea1580450f131c8.
makes sense that `setBlockIndexCandidates` should ideally contain all block indices with as much/more work than current tip regardless of whether it is an ancestor/descendant.
I've tested it by demonstrating a situation where the chain tip doesn't get correctly updated on master - https://github.com/stratospher/bitcoin/commit/57959f9b659f9df13aca3d08b2a14adc9588ac07, feel free to cherry-pick if useful.
💬 stratospher commented on pull request "validation: Add eligible ancestors of reconsidered block to setBlockIndexCandidates":
(https://github.com/bitcoin/bitcoin/pull/30479#discussion_r1968871736)
4a76a83: also update comment in src/validation.h
(https://github.com/bitcoin/bitcoin/pull/30479#discussion_r1968871736)
4a76a83: also update comment in src/validation.h
💬 dongwook-chan commented on pull request "rpc: Support v3 raw transactions creation":
(https://github.com/bitcoin/bitcoin/pull/31936#discussion_r1976352349)
@luke-jr
We had to add it because without it, [RPC method vaildation](https://github.com/bitcoin/bitcoin/blob/a0473442d1c22043f5a288bd9255c006fd85d947/test/functional/rpc_help.py#L71) does not pass.
(https://github.com/bitcoin/bitcoin/pull/31936#discussion_r1976352349)
@luke-jr
We had to add it because without it, [RPC method vaildation](https://github.com/bitcoin/bitcoin/blob/a0473442d1c22043f5a288bd9255c006fd85d947/test/functional/rpc_help.py#L71) does not pass.
💬 chungeun-choi commented on pull request "rpc: Support v3 raw transactions creation":
(https://github.com/bitcoin/bitcoin/pull/31936#issuecomment-2692034130)
@glozow @luke-jr PTAL
(https://github.com/bitcoin/bitcoin/pull/31936#issuecomment-2692034130)
@glozow @luke-jr PTAL
🤔 rkrux reviewed a pull request: "rpc: Support v3 raw transactions creation"
(https://github.com/bitcoin/bitcoin/pull/31936#pullrequestreview-2652336445)
Thank you for working on this, please mention in the PR description that it closes the issue #31348.
(https://github.com/bitcoin/bitcoin/pull/31936#pullrequestreview-2652336445)
Thank you for working on this, please mention in the PR description that it closes the issue #31348.
💬 rkrux commented on pull request "rpc: Support v3 raw transactions creation":
(https://github.com/bitcoin/bitcoin/pull/31936#discussion_r1976355890)
IMO we should add a test wherein a v3 transaction is successfully created, signed, and broadcast to the network. ATM, only the erroneous cases are covered.
(https://github.com/bitcoin/bitcoin/pull/31936#discussion_r1976355890)
IMO we should add a test wherein a v3 transaction is successfully created, signed, and broadcast to the network. ATM, only the erroneous cases are covered.
💬 dongwook-chan commented on pull request "rpc: Support v3 raw transactions creation":
(https://github.com/bitcoin/bitcoin/pull/31936#issuecomment-2692040957)
@glozow @luke-jr @rkrux
While updating the code, I noticed a couple of potential improvements.
1. No range validation tests for `locktime` argument in `createrawtransaction` RPC.
I added range validation tests for new `version` argument. Should we extend this to `locktime` as well?
2. `createpsbt` still creates only v2 raw transactions even though it shares same `RPCHelpMan` with `createrawtransaction`. Should it support v3 raw transactions too?
(https://github.com/bitcoin/bitcoin/pull/31936#issuecomment-2692040957)
@glozow @luke-jr @rkrux
While updating the code, I noticed a couple of potential improvements.
1. No range validation tests for `locktime` argument in `createrawtransaction` RPC.
I added range validation tests for new `version` argument. Should we extend this to `locktime` as well?
2. `createpsbt` still creates only v2 raw transactions even though it shares same `RPCHelpMan` with `createrawtransaction`. Should it support v3 raw transactions too?
💬 rkrux commented on pull request "rpc: Support v3 raw transactions creation":
(https://github.com/bitcoin/bitcoin/pull/31936#issuecomment-2692064976)
Re: https://github.com/bitcoin/bitcoin/pull/31936#issuecomment-2692040957
1. There seem to be couple functional tests related to locktime out of range here: https://github.com/bitcoin/bitcoin/blob/3c1f72a36700271c7c1293383549c3be29f28edb/test/functional/rpc_rawtransaction.py#L312-L313
2. Good point, I think it should but in a separate PR because along with it, we'd need to ensure that other PSBT related RPCs such as `walletcreatefundedpsbt` among others work with v3 transactions as well. T
...
(https://github.com/bitcoin/bitcoin/pull/31936#issuecomment-2692064976)
Re: https://github.com/bitcoin/bitcoin/pull/31936#issuecomment-2692040957
1. There seem to be couple functional tests related to locktime out of range here: https://github.com/bitcoin/bitcoin/blob/3c1f72a36700271c7c1293383549c3be29f28edb/test/functional/rpc_rawtransaction.py#L312-L313
2. Good point, I think it should but in a separate PR because along with it, we'd need to ensure that other PSBT related RPCs such as `walletcreatefundedpsbt` among others work with v3 transactions as well. T
...
💬 rkrux commented on pull request "rpc: Support v3 raw transactions creation":
(https://github.com/bitcoin/bitcoin/pull/31936#issuecomment-2692091752)
> There's a PSBT_GLOBAL_TX_VERSION field as well in the PSBT that might need to be set to 3
An update to my previous comment: https://github.com/bitcoin/bitcoin/pull/31936#issuecomment-2692064976
This can only done when PSBT V2 is implemented https://github.com/bitcoin/bitcoin/pull/21283, so no need for this at the moment.
(https://github.com/bitcoin/bitcoin/pull/31936#issuecomment-2692091752)
> There's a PSBT_GLOBAL_TX_VERSION field as well in the PSBT that might need to be set to 3
An update to my previous comment: https://github.com/bitcoin/bitcoin/pull/31936#issuecomment-2692064976
This can only done when PSBT V2 is implemented https://github.com/bitcoin/bitcoin/pull/21283, so no need for this at the moment.
📝 lurdporch21 opened a pull request: "Pi network alpha "
(https://github.com/bitcoin/bitcoin/pull/31967)
Pi network extending blockchain through Bitcoin and Ethereum network ### _****_
Unlike nodes that use proof of work such as Bitcoin or Ethereum, Pi Node uses a different consensus algorithm based on the Stellar Consensus Protocol (SCP). In SCP, nodes form trusted groups (quorum slices) and only agree to transactions that those trusted nodes agree to. The security circles (See Pi FAQ: What are security circles?) from Pi mobile miners aggregate into a global trust graph that enables Pi Nodes
...
(https://github.com/bitcoin/bitcoin/pull/31967)
Pi network extending blockchain through Bitcoin and Ethereum network ### _****_
Unlike nodes that use proof of work such as Bitcoin or Ethereum, Pi Node uses a different consensus algorithm based on the Stellar Consensus Protocol (SCP). In SCP, nodes form trusted groups (quorum slices) and only agree to transactions that those trusted nodes agree to. The security circles (See Pi FAQ: What are security circles?) from Pi mobile miners aggregate into a global trust graph that enables Pi Nodes
...
✅ lurdporch21 closed a pull request: "Pi network alpha"
(https://github.com/bitcoin/bitcoin/pull/31967)
(https://github.com/bitcoin/bitcoin/pull/31967)
📝 lurdporch21 reopened a pull request: "Pi network alpha"
(https://github.com/bitcoin/bitcoin/pull/31967)
Pi network extending blockchain through Bitcoin and Ethereum network ### _****_
Unlike nodes that use proof of work such as Bitcoin or Ethereum, Pi Node uses a different consensus algorithm based on the Stellar Consensus Protocol (SCP). In SCP, nodes form trusted groups (quorum slices) and only agree to transactions that those trusted nodes agree to. The security circles (See Pi FAQ: What are security circles?) from Pi mobile miners aggregate into a global trust graph that enables Pi Nodes
...
(https://github.com/bitcoin/bitcoin/pull/31967)
Pi network extending blockchain through Bitcoin and Ethereum network ### _****_
Unlike nodes that use proof of work such as Bitcoin or Ethereum, Pi Node uses a different consensus algorithm based on the Stellar Consensus Protocol (SCP). In SCP, nodes form trusted groups (quorum slices) and only agree to transactions that those trusted nodes agree to. The security circles (See Pi FAQ: What are security circles?) from Pi mobile miners aggregate into a global trust graph that enables Pi Nodes
...
✅ fanquake closed a pull request: "PiBtc"
(https://github.com/bitcoin/bitcoin/pull/31967)
(https://github.com/bitcoin/bitcoin/pull/31967)