Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

refactor: use new type of composite commands for protx NNN #6072

Merged
merged 5 commits into from
Jun 27, 2024
Merged

refactor: use new type of composite commands for protx NNN #6072

merged 5 commits into from
Jun 27, 2024

Conversation

knst
Copy link
Collaborator

@knst knst commented Jun 21, 2024

Issue being fixed or feature implemented

See #6051

What was done?

Commands starting from 'protx ...' uses new a new way to make composite commands.

How Has This Been Tested?

Run unit/functional tests.

Breaking Changes

N/A

Checklist:

  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have added or updated relevant unit/integration/functional/e2e tests
  • I have made corresponding changes to the documentation
  • I have assigned this pull request to a milestone

@knst knst added this to the 21 milestone Jun 21, 2024
@knst knst changed the title refactor: use new type of composite commands for protx NNN refactor: use new type of composite commands for protx NNN Jun 21, 2024
@knst knst force-pushed the refactor-rpc-18531-p4 branch from 82e524d to e96776a Compare June 21, 2024 19:48
@knst knst requested review from UdjinM6 and PastaPastaPasta June 22, 2024 04:58
UdjinM6
UdjinM6 previously approved these changes Jun 24, 2024
View reviewed changes
Copy link

@UdjinM6 UdjinM6 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK

@github-actions GitHub Actions
Copy link

This pull request has conflicts, please rebase.

@knst knst force-pushed the refactor-rpc-18531-p4 branch from e96776a to 7b44af2 Compare June 25, 2024 15:37
@knst knst requested a review from UdjinM6 June 25, 2024 15:38
UdjinM6
UdjinM6 approved these changes Jun 25, 2024
View reviewed changes
Copy link

@UdjinM6 UdjinM6 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

re-utACK

PastaPastaPasta
PastaPastaPasta reviewed Jun 27, 2024
View reviewed changes
Copy link
Member

@PastaPastaPasta PastaPastaPasta left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK 7b44af2

@PastaPastaPasta PastaPastaPasta merged commit b0426f3 into dashpay:develop Jun 27, 2024
8 of 9 checks passed
@knst knst deleted the refactor-rpc-18531-p4 branch June 29, 2024 09:07
knst added a commit to knst/dash that referenced this pull request Dec 26, 2024
@knst
fix: undefined behaviour due to released memory after call of RPCHelpMan
c546b0e 
The implementation of RPC has been refactored in bitcoin significantly with using RPCHelpMan,
see multiple PR, such as bitcoin#1853, bitcoin#19528, etc

Backporting them and appliying same refactoring to Dash Core caused undefined behaviour, particularly dashpay#6072

For example, in this code the local variable `use_legacy` will be used after `protx_update_registrar_wrapper`,
when executor will be called.

    static RPCHelpMan protx_update_registrar_wrapper(const bool use_legacy)
    {
        std::string rpc_name = use_legacy ? "update_registrar_legacy" : "update_registrar";
        std::string rpc_full_name = std::string("protx ").append(rpc_name);
        std::string pubkey_operator = use_legacy ? "\"0532646990082f4fd639f90387b1551f2c7c39d37392cb9055a06a7e85c1d23692db8f87f827886310bccc1e29db9aee\"" : "\"8532646990082f4fd639f90387b1551f2c7c39d37392cb9055a06a7e85c1d23692db8f87f827886310bccc1e29db9aee\"";
        std::string rpc_example = rpc_name.append(" \"0123456701234567012345670123456701234567012345670123456701234567\" ").append(pubkey_operator).append(" \"" + EXAMPLE_ADDRESS[1] + "\"");
        return RPCHelpMan{rpc_full_name,
        <...>
        RPCResult{
            RPCResult::Type::STR_HEX, "txid", "The transaction id"
        },
        RPCExamples{
            HelpExampleCli("protx", rpc_example)
        },
        [&](const RPCHelpMan& self, const JSONRPCRequest& request) -> UniValue
    {
        <...>
        ptx.nVersion = specific_legacy_bls_scheme ? CProUpRegTx::LEGACY_BLS_VERSION : CProUpRegTx::BASIC_BLS_VERSION; <<<<---- THERE IS UB

It can be easy tested by adding debug logs to log string, for example rpc_full_name at the moment of call of executor has been during run:

    2024-12-26T16:10:15Z rpc full name: 'r,ߧzu\x00\x00����m���istrar_legacy'

This PR fixes multiple unexplainable random failures which had happen only for `tsan` build or for only `ubsan` build during debuggin dashpay#6508
and depends only on code revision.
knst added a commit to knst/dash that referenced this pull request Dec 26, 2024
@knst
fix: undefined behaviour in evo RPC calls of RPCHelpMan
7f212db 
The implementation of RPC has been refactored in bitcoin significantly with using RPCHelpMan,
see multiple PR, such as bitcoin#1853, bitcoin#19528, etc

Backporting them and appliying same refactoring to Dash Core caused undefined behaviour, particularly dashpay#6072

For example, in this code the local variable `use_legacy` will be used after `protx_update_registrar_wrapper`,
when executor will be called.

    static RPCHelpMan protx_update_registrar_wrapper(const bool use_legacy)
    {
        std::string rpc_name = use_legacy ? "update_registrar_legacy" : "update_registrar";
        std::string rpc_full_name = std::string("protx ").append(rpc_name);
        std::string pubkey_operator = use_legacy ? "\"0532646990082f4fd639f90387b1551f2c7c39d37392cb9055a06a7e85c1d23692db8f87f827886310bccc1e29db9aee\"" : "\"8532646990082f4fd639f90387b1551f2c7c39d37392cb9055a06a7e85c1d23692db8f87f827886310bccc1e29db9aee\"";
        std::string rpc_example = rpc_name.append(" \"0123456701234567012345670123456701234567012345670123456701234567\" ").append(pubkey_operator).append(" \"" + EXAMPLE_ADDRESS[1] + "\"");
        return RPCHelpMan{rpc_full_name,
        <...>
        RPCResult{
            RPCResult::Type::STR_HEX, "txid", "The transaction id"
        },
        RPCExamples{
            HelpExampleCli("protx", rpc_example)
        },
        [&](const RPCHelpMan& self, const JSONRPCRequest& request) -> UniValue
    {
        <...>
        ptx.nVersion = specific_legacy_bls_scheme ? CProUpRegTx::LEGACY_BLS_VERSION : CProUpRegTx::BASIC_BLS_VERSION; <<<<---- THERE IS UB

It can be easy tested by adding debug logs to log string, for example rpc_full_name at the moment of call of executor has been during run:

    2024-12-26T16:10:15Z rpc full name: 'r,ߧzu\x00\x00����m���istrar_legacy'

This PR fixes multiple unexplainable random failures which had happen only for `tsan` build or for only `ubsan` build during debuggin dashpay#6508
and depends only on code revision.
knst added a commit to knst/dash that referenced this pull request Dec 26, 2024
@knst
fix: undefined behaviour in evo RPC calls of RPCHelpMan
665178f 
The implementation of RPC has been refactored in bitcoin significantly with using RPCHelpMan,
see multiple PR, such as bitcoin#1853, bitcoin#19528, etc

Backporting them and appliying same refactoring to Dash Core caused undefined behaviour, particularly dashpay#6072

For example, in this code the local variable `use_legacy` will be used after `protx_update_registrar_wrapper`,
when executor will be called.

    static RPCHelpMan protx_update_registrar_wrapper(const bool use_legacy)
    {
        std::string rpc_name = use_legacy ? "update_registrar_legacy" : "update_registrar";
        std::string rpc_full_name = std::string("protx ").append(rpc_name);
        std::string pubkey_operator = use_legacy ? "\"0532646990082f4fd639f90387b1551f2c7c39d37392cb9055a06a7e85c1d23692db8f87f827886310bccc1e29db9aee\"" : "\"8532646990082f4fd639f90387b1551f2c7c39d37392cb9055a06a7e85c1d23692db8f87f827886310bccc1e29db9aee\"";
        std::string rpc_example = rpc_name.append(" \"0123456701234567012345670123456701234567012345670123456701234567\" ").append(pubkey_operator).append(" \"" + EXAMPLE_ADDRESS[1] + "\"");
        return RPCHelpMan{rpc_full_name,
        <...>
        RPCResult{
            RPCResult::Type::STR_HEX, "txid", "The transaction id"
        },
        RPCExamples{
            HelpExampleCli("protx", rpc_example)
        },
        [&](const RPCHelpMan& self, const JSONRPCRequest& request) -> UniValue
    {
        <...>
        ptx.nVersion = specific_legacy_bls_scheme ? CProUpRegTx::LEGACY_BLS_VERSION : CProUpRegTx::BASIC_BLS_VERSION; <<<<---- THERE IS UB

It can be easy tested by adding debug logs to log string, for example rpc_full_name at the moment of call of executor has been during run:

    2024-12-26T16:10:15Z rpc full name: 'r,ߧzu\x00\x00����m���istrar_legacy'

This PR fixes multiple unexplainable random failures which had happen only for `tsan` build or for only `ubsan` build during debuggin dashpay#6508
and depends only on code revision.
knst added a commit to knst/dash that referenced this pull request Dec 26, 2024
@knst
fix: undefined behaviour in evo RPC calls of RPCHelpMan
1cabb75 
The implementation of RPC has been refactored in bitcoin significantly with using RPCHelpMan,
see multiple PR, such as bitcoin#1853, bitcoin#19528, etc

Backporting them and appliying same refactoring to Dash Core caused undefined behaviour, particularly dashpay#6072

For example, in this code the local variable `use_legacy` will be used after `protx_update_registrar_wrapper`,
when executor will be called.

    static RPCHelpMan protx_update_registrar_wrapper(const bool use_legacy)
    {
        std::string rpc_name = use_legacy ? "update_registrar_legacy" : "update_registrar";
        std::string rpc_full_name = std::string("protx ").append(rpc_name);
        std::string pubkey_operator = use_legacy ? "\"0532646990082f4fd639f90387b1551f2c7c39d37392cb9055a06a7e85c1d23692db8f87f827886310bccc1e29db9aee\"" : "\"8532646990082f4fd639f90387b1551f2c7c39d37392cb9055a06a7e85c1d23692db8f87f827886310bccc1e29db9aee\"";
        std::string rpc_example = rpc_name.append(" \"0123456701234567012345670123456701234567012345670123456701234567\" ").append(pubkey_operator).append(" \"" + EXAMPLE_ADDRESS[1] + "\"");
        return RPCHelpMan{rpc_full_name,
        <...>
        RPCResult{
            RPCResult::Type::STR_HEX, "txid", "The transaction id"
        },
        RPCExamples{
            HelpExampleCli("protx", rpc_example)
        },
        [&](const RPCHelpMan& self, const JSONRPCRequest& request) -> UniValue
    {
        <...>
        ptx.nVersion = specific_legacy_bls_scheme ? CProUpRegTx::LEGACY_BLS_VERSION : CProUpRegTx::BASIC_BLS_VERSION; <<<<---- THERE IS UB

It can be easy tested by adding debug logs to log string, for example rpc_full_name at the moment of call of executor has been during run:

    2024-12-26T16:10:15Z rpc full name: 'r,ߧzu\x00\x00����m���istrar_legacy'

This PR fixes multiple unexplainable random failures which had happen only for `tsan` build or for only `ubsan` build during debuggin dashpay#6508
and depends only on code revision.
knst added a commit to knst/dash that referenced this pull request Dec 27, 2024
@knst
fix: undefined behaviour in evo RPC calls of RPCHelpMan
954e5ef 
The implementation of RPC has been refactored in bitcoin significantly with using RPCHelpMan,
see multiple PR, such as bitcoin#1853, bitcoin#19528, etc

Backporting them and appliying same refactoring to Dash Core caused undefined behaviour, particularly dashpay#6072

For example, in this code the local variable `use_legacy` will be used after `protx_update_registrar_wrapper`,
when executor will be called.

    static RPCHelpMan protx_update_registrar_wrapper(const bool use_legacy)
    {
        std::string rpc_name = use_legacy ? "update_registrar_legacy" : "update_registrar";
        std::string rpc_full_name = std::string("protx ").append(rpc_name);
        std::string pubkey_operator = use_legacy ? "\"0532646990082f4fd639f90387b1551f2c7c39d37392cb9055a06a7e85c1d23692db8f87f827886310bccc1e29db9aee\"" : "\"8532646990082f4fd639f90387b1551f2c7c39d37392cb9055a06a7e85c1d23692db8f87f827886310bccc1e29db9aee\"";
        std::string rpc_example = rpc_name.append(" \"0123456701234567012345670123456701234567012345670123456701234567\" ").append(pubkey_operator).append(" \"" + EXAMPLE_ADDRESS[1] + "\"");
        return RPCHelpMan{rpc_full_name,
        <...>
        RPCResult{
            RPCResult::Type::STR_HEX, "txid", "The transaction id"
        },
        RPCExamples{
            HelpExampleCli("protx", rpc_example)
        },
        [&](const RPCHelpMan& self, const JSONRPCRequest& request) -> UniValue
    {
        <...>
        ptx.nVersion = specific_legacy_bls_scheme ? CProUpRegTx::LEGACY_BLS_VERSION : CProUpRegTx::BASIC_BLS_VERSION; <<<<---- THERE IS UB

It can be easy tested by adding debug logs to log string, for example rpc_full_name at the moment of call of executor has been during run:

    2024-12-26T16:10:15Z rpc full name: 'r,ߧzu\x00\x00����m���istrar_legacy'

This PR fixes multiple unexplainable random failures which had happen only for `tsan` build or for only `ubsan` build during debuggin dashpay#6508
and depends only on code revision.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@PastaPastaPasta PastaPastaPasta PastaPastaPasta left review comments

@UdjinM6 UdjinM6 UdjinM6 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
21
Development

Successfully merging this pull request may close these issues.
3 participants
@knst @UdjinM6 @PastaPastaPasta