[WIP][RPC] RingCT output overhaul #688

mimirmim · 2019-11-19T14:48:26Z

Problem

RPC needs to be expanded to allow for key images to be revealed. This PR is meant to give the RPC the necessary outputs in the proper locations to allow us to extend the RingCT protocol of Veil outside of Veil Core. This is required for future Veil prospects such as Veil "Link" server and client as it needs to be able to get the key_images.

Root Cause

It never has been included in the RPC before, even with the inherited code.

Solution

Adding it to the method that allows us to serialise our transactional data to JSON.

Testing Results

Following RPC commands were update:
getrawtransaction
decoderawtransaction
getblock

This builds upon #686 by @codeofalltrades.

Example output:

    {
      "type": "ringct",
      "num_inputs": 5,
      "ring_size": 11,
      "commitment_sig": "0ab0dcd6bed386ce73b17ea89627a5a02b0912a8fe063c02c68557c1f96ebcf0",
      "ringct_inputs": [
        {
          "key_image": "020b2bc3bce9478f80c45b08b48019ea104e2b24d53aa15b8ffa6c0d7b45250c27",
          "txid": "6b3d6e10dbe9e4088da2e2fc3b31f1268addc19accc3bd2388ce172d7f600103",
          "index": 67903,
          "vout.n": 10,
          "commitments": [
            "854e5b70dd18b909ebdf644f483ad94c52584ec13f9c3c3d2951840eebde0ccf",
            "c88199117bbb830b8c17da8570e7b48135323f40418facaf3befedb8d8970a3a",
            "03e3b8be34b0d643b1556d8bf17766433c5f83277066b3aada512a21f1de4b59",
            "42111786d0788340f7077410185d8e1cdc04bab409594c8fc778c44fa9b36426",
            "872f33f39e6f8c76c62ce978cef93fe9a540437079bd0752f50b9b914c3091bf",
            "89f01e9602af691014a8b4c7211ccb90846fd20ccdd94a495b671e29ed2875fc",
            "7a85cb2df1c985c298ea54db6f99dc3dce9a6f89dc6d9ff6fc0eecb5f096cd3f",
            "8c9b7620f20e1bb9e262709744680036aa36e272f49728612c7b5a18037cb21c",
            "bada51097958fe3d47090b27a49faf85aff9af16da202d8bc1c2995310b1790a",
            "8fdd66018a3f1932bdaf353bd9b7201a6f35992c4785c55d3b553469956f88dd",
            "0dacc7e1a6050c0eef99594ba959377a2b904dc99eb5232593555ee4c2122971"
          ]
        },
        .  .  .

Additional

The code that is in the debug section of the JSON output of gettransaction was deleted to conform with standard Bitcoin RPC protocol.

This debug information was only temporarily added to check out some stuff.

Cleaner to add in an overload method than to pass in anything empty. Lots of duplicate code so this absolutely can be improved greater than this. The reason for this is that veil-tx and some test files will fail to build if `GetTxRingCtInputs`is there.

This should be faster to do it like this. I'm not sure why the other class was considered.

We need the RPC to be able to display the key images for RingCT. This allows us to extend Veil more beyond just Veil Core.

codeofalltrades

ACK 0eac106
A few trivial changes and noted todos.

src/rpc/rawtransaction.cpp

src/rest.cpp

src/core_write.cpp

src/core_io.h

src/rpc/rawtransaction.cpp

src/veil-tx.cpp

src/veil/ringct/anon.cpp

src/veil/ringct/anon.h

src/veil/ringct/rpcanonwallet.cpp

src/wallet/rpcwallet.cpp

Copyrights added to changed flies and commented line removed as per review request.

codeofalltrades

ACK 3abedee

The count up from 0 is too much and the map itself should've been sequential. Erasing the map and putting it all together as an array makes a lot more sense.

`data_hex` in this context is infact the ephemeral public key. Best to name it as such. Also, the `objKeyImage` is in fact `arrKeyImage`. Missed that on my go through.

codeofalltrades

ACK 9cd1242
Once these changes are final we should rebase to one commit.

CaveSpectre11

utACK - Definitely rebase when merging so this can be cherry picked more easily

Visually, it didn't look great at all to lump all the commitment, inputs, and key_images together when they are related to one another in groups. This is meant to visually represent that accurately. This should make it easier to turn the Json into objects from other clients and make representation in explorers better.

codeofalltrades

ACK f6ea7a1
rebase would be the only other item.

codeofalltrades · 2019-11-29T15:03:50Z

src/core_write.cpp

            UniValue o(UniValue::VOBJ);
            ScriptPubKeyToUniv(s->scriptPubKey, o, true);
            entry.pushKV("scriptPubKey", o);
-            entry.pushKV("data_hex", HexStr(s->vData.begin(), s->vData.end()));
+            entry.pushKV("valueCommitment", HexStr(&s->commitment.data[0], &s->commitment.data[0]+33));


We will want to make 33 at const as some point?

a const for 33 is added as part of #696

codeofalltrades · 2019-11-29T15:05:02Z

src/core_write.cpp

+            const std::vector<uint8_t> vKeyImages = txin.scriptData.stack[0];
+            UniValue arrKeyImage(UniValue::VARR);
+            for (unsigned int k = 0; k < nSigInputs; k++) {
+                arrKeyImage.push_back(HexStr(&vKeyImages[k*33], &vKeyImages[(k*33)+33]));


We will want to make 33 at const as some point?

mimirmim · 2020-01-19T15:02:06Z

I'll get this rebased once I wake up. This has to make it into the 2.0 branch as it will break RPC backwards compatibility.

CaveSpectre11 · 2020-01-20T13:24:45Z

This has to make it into the 2.0 branch as it will break RPC backwards compatibility.

In that case we should create a 2.0 branch and revise the PR to be a merge request into the 2.0 branch; so it doesn't accidentally get pushed into master. Probably a good thing to do in conjunction with the rebase and conflict resolution.

CaveSpectre11 · 2020-01-20T13:55:43Z

This has to make it into the 2.0 branch as it will break RPC backwards compatibility.

I have been going through again trying to find where this won't be compatible with the current chain, but I can't pinpoint exactly. We will need to tie that portion to the hardfork marker; since I believe the intention is to hardfork via rejecting old protocols a certain amount of time after release, to allow for enough 2.0 nodes to be upgraded.

e.g.
if (New_Protocol_Enforcement)
do the new stuff
else
support the old protocol

mimirmim · 2020-01-25T06:37:48Z

We talked about this, it isn't about chain incompatibility.

The software implements the Veil protocol which has it's own versioning (protocol version). We can have a hard fork and the software wouldn't go up a major version as its own RPC API is still backwards compatible.

This is a major change to the RPC API to provide more for external software that needs to build independent RingCT transactions. Thus, breaking compatibility for current users of our RPC API for that method.

mimirmim · 2020-03-17T18:52:25Z

This is going back to WIP until I can fix it up and change somethings post Veil RingCT Staking.

seanPhill · 2022-12-21T15:09:49Z

@Zannick This should have been committed a long time ago. Can you please review this in the light of your RingCT PR, and we'll see if we can test both PRs together?

codeofalltrades and others added 5 commits November 18, 2019 11:27

Add key_image value(s) to gettransaction rpc output

0eac106

Remove unneeded debug information from gettransaction

88a6f40

This debug information was only temporarily added to check out some stuff.

GetTxringCtInputs takes in an actual reference

f281df7

This should be faster to do it like this. I'm not sure why the other class was considered.

KeyImage code added to TxToUniv

480aa78

We need the RPC to be able to display the key images for RingCT. This allows us to extend Veil more beyond just Veil Core.

mimirmim requested review from blondfrogs, CaveSpectre11 and codeofalltrades November 19, 2019 14:48

mimirmim added Component: RPC Related to the console commands themselves. Tag: Waiting For Code Review Waiting for code review from a core developer labels Nov 19, 2019

mimirmim force-pushed the rpc-fixes branch from 595a7c2 to 480aa78 Compare November 19, 2019 15:58

mimirmim mentioned this pull request Nov 19, 2019

RPC Calls Need to be Checked and Fixed for RingCT Integration #682

Open

25 tasks

codeofalltrades requested changes Nov 19, 2019

View reviewed changes

Copyright and cleanup

3abedee

Copyrights added to changed flies and commented line removed as per review request.

mimirmim requested a review from codeofalltrades November 20, 2019 17:42

codeofalltrades approved these changes Nov 20, 2019

View reviewed changes

Change key images from array sequence map to array.

1e699d7

The count up from 0 is too much and the map itself should've been sequential. Erasing the map and putting it all together as an array makes a lot more sense.

mimirmim requested a review from codeofalltrades November 21, 2019 10:22

Correct naming for array and ephemeral public key

9cd1242

`data_hex` in this context is infact the ephemeral public key. Best to name it as such. Also, the `objKeyImage` is in fact `arrKeyImage`. Missed that on my go through.

codeofalltrades approved these changes Nov 22, 2019

View reviewed changes

CaveSpectre11 approved these changes Nov 24, 2019

View reviewed changes

mimirmim changed the title ~~[RPC] Key images to RPC and general code clean up~~ [RPC] RingCT output overhaul Nov 29, 2019

codeofalltrades approved these changes Nov 29, 2019

View reviewed changes

mimirmim added Code Review: Passed and removed Tag: Waiting For Code Review Waiting for code review from a core developer labels Dec 3, 2019

mimirmim added this to the v2.0.0 milestone Dec 4, 2019

CaveSpectre11 assigned mimirmim Dec 18, 2019

CaveSpectre11 added the Needs Rebase PR needs to be rebased against current master label Jan 17, 2020

mimirmim added Dev Status: In Progress Someone is actively working on this issue. and removed Needs Rebase PR needs to be rebased against current master labels Mar 17, 2020

mimirmim changed the title ~~[RPC] RingCT output overhaul~~ [WIP][RPC] RingCT output overhaul Mar 17, 2020

CaveSpectre11 removed the Code Review: Passed label Mar 18, 2020

seanPhill requested a review from Zannick December 21, 2022 15:07

seanPhill added the Coin Type: RingCT Specifically related to RingCT transactions label Dec 21, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP][RPC] RingCT output overhaul #688

[WIP][RPC] RingCT output overhaul #688

mimirmim commented Nov 19, 2019 •

edited

Loading

codeofalltrades left a comment

codeofalltrades left a comment

codeofalltrades left a comment

CaveSpectre11 left a comment

codeofalltrades left a comment

codeofalltrades Nov 29, 2019

CaveSpectre11 Dec 2, 2019

codeofalltrades Nov 29, 2019

mimirmim commented Jan 19, 2020

CaveSpectre11 commented Jan 20, 2020

CaveSpectre11 commented Jan 20, 2020

mimirmim commented Jan 25, 2020 •

edited

Loading

mimirmim commented Mar 17, 2020

seanPhill commented Dec 21, 2022

[WIP][RPC] RingCT output overhaul #688

Are you sure you want to change the base?

[WIP][RPC] RingCT output overhaul #688

Conversation

mimirmim commented Nov 19, 2019 • edited Loading

Problem

Root Cause

Solution

Testing Results

Additional

codeofalltrades left a comment

Choose a reason for hiding this comment

codeofalltrades left a comment

Choose a reason for hiding this comment

codeofalltrades left a comment

Choose a reason for hiding this comment

CaveSpectre11 left a comment

Choose a reason for hiding this comment

codeofalltrades left a comment

Choose a reason for hiding this comment

codeofalltrades Nov 29, 2019

Choose a reason for hiding this comment

CaveSpectre11 Dec 2, 2019

Choose a reason for hiding this comment

codeofalltrades Nov 29, 2019

Choose a reason for hiding this comment

mimirmim commented Jan 19, 2020

CaveSpectre11 commented Jan 20, 2020

CaveSpectre11 commented Jan 20, 2020

mimirmim commented Jan 25, 2020 • edited Loading

mimirmim commented Mar 17, 2020

seanPhill commented Dec 21, 2022

mimirmim commented Nov 19, 2019 •

edited

Loading

mimirmim commented Jan 25, 2020 •

edited

Loading