fix: creditOutputs in AssetLock tx json output should be an array of objects, not debug strings

[UdjinM6]

Issue being fixed or feature implemented

Txout-s in creditOutputs for AssetLock txes should be shown the way txout-s are shown in other places. We should not be using debug strings there.

Example: getrawtransaction 50757f651f335e22c5a810bd05c1e5aac0d95b132f6454e2a72683f88e3983f3 1

develop:

  "assetLockTx": {
    "version": 1,
    "creditOutputs": [
      "CTxOut(nValue=0.01000000, scriptPubKey=76a914cdfca4ae1cf2333056659a2c)"
    ]
  },

This PR:

  "assetLockTx": {
    "version": 1,
    "creditOutputs": [
      {
        "value": 0.01000000,
        "valueSat": 1000000,
        "scriptPubKey": {
          "asm": "OP_DUP OP_HASH160 cdfca4ae1cf2333056659a2c8dc656f36d228402 OP_EQUALVERIFY OP_CHECKSIG",
          "hex": "76a914cdfca4ae1cf2333056659a2c8dc656f36d22840288ac",
          "address": "yf6c2VSpWGXUgmjQSHRpfEcTPsbqN4oL4c",
          "type": "pubkeyhash"
        }
      }
    ]
  },

kudos to @coolaj86 for finding the issue

What was done?

Change CAssetLockPayload::ToJson() output to be closer to TxToUniv()

NOTE: refactor: use key_to_p2pkh_script in more places commit is a bit unrelated but I decided to add it anyway to make it easier to follow assetlock creation vs getrawtransaction rpc check.

How Has This Been Tested?

Try example above, run tests

Breaking Changes

RPC output is different for AssetLock txes

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

[coolaj86]

Thank you @UdjinM6!

[PastaPastaPasta]

This is a breaking change, also not a fix really but a feat I guess? As such, can't merge in pre-v22, cannot backport

we could merge in e54fe42 earlier ofc

[UdjinM6]

In no other places (in RPC) txouts are shown like that. The data is incomplete so you could get the same scriptPubKey string for two different actual scriptPubKey-s. If you use that for some logic in your app things will get messy one day. Basically, the way it works right now makes RPC output unusable/dangerous which is clearly a bug that must be fixed asap imo.

[knst]

could you add here a forward declaration of ScriptPubKeyToUniv or do something else?

To avoid:

"core_io -> evo/assetlocktx -> core_io"

[PastaPastaPasta]

In no other places (in RPC) txouts are shown like that. The data is incomplete so you could get the same scriptPubKey string for two different actual scriptPubKey-s. If you use that for some logic in your app things will get messy one day. Basically, the way it works right now makes RPC output unusable/dangerous which is clearly a bug that must be fixed asap imo.

Is it truly inherently broken? I see no reason why platform for example (or anyone else) couldn't be using / relying on it as it right now.

Can you explain further how/why it is incomplete / inherently broken?

[UdjinM6]

Can you explain further how/why it is incomplete / inherently broken?

pls see PR description, tl;dr:
scriptPubKey shown in RPC results in develop:
76a914cdfca4ae1cf2333056659a2c
actual scriptPubKey:
76a914cdfca4ae1cf2333056659a2c8dc656f36d22840288ac

[knst]

utACK b330318

[PastaPastaPasta]

But it's still a breaking change @UdjinM6, platform could be parsing the amount component CTxOut(nValue=0.01000000 and using that somehow. We could ask @shumkov to review and if they sign off on this not being a breaking change for them, then I guess we could merge it in (still concerns me though because still stuff like platform-explorer, mnowatch, misc. explorer could be using it), but generally, we need to be very careful about merging stuff that could in theory break platform.

[shumkov]

It doesn't look like a breaking change for Drive, since it uses only height and hex.

I herby temporarily release you from your obligation to block all PRs which result in breaking changes in minor versions. This temporary release shall only be for the PR 6229. I herby solemnly swear to release you from all potential blame arising from any issues related to platform as a result of merging this PR.

[PastaPastaPasta]

@UdjinM6 I think with release notes documenting this change, I can merge it for 21.2

[PastaPastaPasta]

utACK 9876c2d

[thephez]

Good thing these messages have an edit log @shumkov 😂

It doesn't look like a breaking change for Drive, since it uses only height and hex.

I herby temporarily release you from your obligation to block all PRs which result in breaking changes in minor versions. This temporary release shall only be for the PR 6229. I herby solemnly swear to release you from all potential blame arising from any issues related to platform as a result of merging this PR.