scravy commented 5 years ago

Everything RPC!

This adds three new RPC commands tracechain, tracestake, and getstakeablecoins. It also cleans up a lot of RPC stuff which has been missed in the past:

maintain the list of rpc command files in check-rpc-mappings.py
parse utf-8/unicode in check-rpc-mappings.py
format dispatch tables accordingly.
parse alternative format in check-rpc-mappings.py
proper help texts for proposer* rpcs
proper RPC_* error messages, e.g. when still in warmup
streamline injected *RPC components using GetComponent

The new commands do the following:

`tracechain`

Starting from start (defaults to the current tip of the active chain) for length number of blocks: Print coinbase information like stake, reward, combined stake, returned stake. For genesis block: Show initial funds.

`tracestake`

This one allows you to inspect the chain and whether all the stakes refer to the right utxo's.

`getstakeablecoins`

Enumerates explicitly the coins which are available for staking. Very useful in functional tests and to debug.

How to test / How to use

$ mkdir something
$ src/unit-e -datadir=something -regtest -printtoconsole -debug=rpc

`getstakeablecoins`

$ src/unit-e-cli -datadir=something -regtest getstakeablecoins
{
  "stakeable_balance": 0.00000000,
  "stakeable_coins": [
  ]
}
$ src/unit-e-cli -datadir=something -regtest importmasterkey 'verify fetch fall amount vast exercise spike they denial weapon maple useful spell goose equip summer before train cheese grace midnight sausage pony bitter'
{
  "wallet": "wallet.dat",
  "language": "English",
  "language_tag": "english",
  "bip39_seed": "17ad058d147ddb06ed53df713b4afe2e6a250c7835122b3a2a0c52e233b37e6b19af90eeb9492361464b9d0cf6cefccfa228e1ca2a361a35b39ee8c4e05b26e0",
  "bip32_root": "tprv8ZgxMBicQKsPeuCrNVUnFkwjhFDcDUn3e9f8RYGaDHNMvV8UQUY5ZBdK1EZFYkgR6FLYjjpSRhAjeiLUVusZMohrQqTjuV11Yjx7D8mMh2W",
  "warnings": [
  ],
  "success": true
}
$ src/unit-e-cli -datadir=something -regtest getstakeablecoins
{
  "stakeable_balance": 10000.00000000,
  "stakeable_coins": [
    {
      "coin": {
        "amount": 10000.00000000,
        "script_pub_key": {
          "asm": "0 916112535557149be624bd284ee2fc96894137f4",
          "hex": "0014916112535557149be624bd284ee2fc96894137f4",
          "reqSigs": 1,
          "type": "witness_v0_keyhash",
          "addresses": [
            "uert1qj9s3y5642u2fhe3yh55yachuj6y5zdl5u30wsd"
          ]
        },
        "out_point": {
          "txid": "ff84e339f368e08686ca333b14cb8889258a587d6d7d016b4918698da4337ae0",
          "n": 8
        }
      },
      "source_block": {
        "height": 0,
        "hash": "5da6150ea6ddadf8e7fcf58a80653c4d289b8e619ab745f5233f615d267caf07",
        "time": 1296688600
      }
    }
  ]
}

`tracestake`

$ src/unit-e-cli -datadir=something -regtest getnewaddress '' bech32
uert1q5zgn0av937xpkvyx7sv52st77mtxn3zjjtmgd2
$ src/unit-e-cli -datadir=something -regtest generatetoaddress 1 uert1q5zgn0av937xpkvyx7sv52st77mtxn3zjjtmgd2
[
  "1e08adf27ccdaccbfbe6f728570c008e914815eeaaa6d7482c814b06557bedde"
]
$ src/unit-e-cli -datadir=something -regtest tracestake
[
  {
    "block_hash": "5da6150ea6ddadf8e7fcf58a80653c4d289b8e619ab745f5233f615d267caf07",
    "block_height": 0,
    "status": "genesis block"
  },
  {
    "block_hash": "1e08adf27ccdaccbfbe6f728570c008e914815eeaaa6d7482c814b06557bedde",
    "block_height": 1,
    "stake_txin": {
      "prevout": {
        "txid": "ff84e339f368e08686ca333b14cb8889258a587d6d7d016b4918698da4337ae0",
        "n": 8
      },
      "scriptSig": {
        "asm": "",
        "hex": ""
      },
      "scriptWitness": [
        "3045022100c32d7d8aed62e7c76e35c33d67e1c760763ae9572f05c01e941070187ddd006202206188dc2d6b8ce357b98556f518108c7865329c813662100c9a8fb05f87d2b4ff01",
        "025c22159e98d63419f1b2c73fc54e6400564a9cd85ba1776aa217f810f0fb39d7"
      ]
    },
    "status": "on disk, spending stake tx not found"
  }
]
$ src/unit-e-cli -datadir=something -regtest generatetoaddress 1 uert1q5zgn0av937xpkvyx7sv52st77mtxn3zjjtmgd2
[
  "1145d68d90a722e492ccaab6edc8ed5e583ef7835488be83c7592a76d8a4d55b"
]
$ src/unit-e-cli -datadir=something -regtest tracestake
[
  {
    "block_hash": "5da6150ea6ddadf8e7fcf58a80653c4d289b8e619ab745f5233f615d267caf07",
    "block_height": 0,
    "status": "genesis block"
  },
  {
    "block_hash": "1e08adf27ccdaccbfbe6f728570c008e914815eeaaa6d7482c814b06557bedde",
    "block_height": 1,
    "stake_txin": {
      "prevout": {
        "txid": "ff84e339f368e08686ca333b14cb8889258a587d6d7d016b4918698da4337ae0",
        "n": 8
      },
      "scriptSig": {
        "asm": "",
        "hex": ""
      },
      "scriptWitness": [
        "3045022100c32d7d8aed62e7c76e35c33d67e1c760763ae9572f05c01e941070187ddd006202206188dc2d6b8ce357b98556f518108c7865329c813662100c9a8fb05f87d2b4ff01",
        "025c22159e98d63419f1b2c73fc54e6400564a9cd85ba1776aa217f810f0fb39d7"
      ]
    },
    "status": "on disk, spending stake tx not found"
  },
  {
    "funding_block_hash": "1e08adf27ccdaccbfbe6f728570c008e914815eeaaa6d7482c814b06557bedde",
    "funding_block_height": 1,
    "block_hash": "1145d68d90a722e492ccaab6edc8ed5e583ef7835488be83c7592a76d8a4d55b",
    "block_height": 2,
    "stake_txout": {
      "amount": 1000.00000000,
      "scriptPubKey": {
        "asm": "0 a09137f5858f8c1b3086f41945417ef6d669c452",
        "hex": "0014a09137f5858f8c1b3086f41945417ef6d669c452",
        "reqSigs": 1,
        "type": "witness_v0_keyhash",
        "addresses": [
          "uert1q5zgn0av937xpkvyx7sv52st77mtxn3zjjtmgd2"
        ]
      }
    },
    "stake_txin": {
      "prevout": {
        "txid": "f12babea21e5d9538ff3ce8dc97b6b56841d788e70fdd5b5945d16a08f57c893",
        "n": 1
      },
      "scriptSig": {
        "asm": "",
        "hex": ""
      },
      "scriptWitness": [
        "304402200ba41dffedefcc0792fd2ae44b7a0b464f66434e19edaa6eee8f3cca2dda3fa102202a79505a8af5888f755467d0917e52c2fd4dd195ab5e7da2eaba23d48af5dc1001",
        "0239806c96d4f027a9fcc62f4b1c14ee903608134679aa4d1de3308d17c99a66b6"
      ]
    },
    "status": "ondisk, stake found"
  }
]

scravy commented 5 years ago

In case you are wondering about 94357bde7c484e8466f8e5b6906c908f9c8e49c6: I organized some includes in order for this PR to not introduce any further circular dependencies. Here's a report:

circular dependencies on master (62) dependencies-master.txt
circular dependencies on this branch (55) dependencies-branch.txt

4c4
< Circular dependency: blockchain/blockchain_rpc -> rpc/util -> injector -> blockchain/blockchain_rpc
---
> Circular dependency: blockdb -> validation -> injector -> blockdb
11,14d10
< Circular dependency: consensus/tx_verify -> finalization/vote_recorder -> injector -> p2p/finalizer_commits_handler -> p2p/finalizer_commits_handler_impl -> consensus/tx_verify
< Circular dependency: consensus/tx_verify -> finalization/vote_recorder -> injector -> proposer/multiwallet -> wallet/wallet -> wallet/walletdb -> consensus/tx_verify
< Circular dependency: consensus/tx_verify -> finalization/vote_recorder -> injector -> staking/transactionpicker -> miner -> consensus/tx_verify
< Circular dependency: consensus/tx_verify -> finalization/vote_recorder -> injector -> staking/transactionpicker -> miner -> txmempool -> consensus/tx_verify
16,18d11
< Circular dependency: esperanza/checks -> finalization/vote_recorder -> injector -> p2p/finalizer_commits_handler -> p2p/finalizer_commits_handler_impl -> esperanza/checks
< Circular dependency: esperanza/checks -> finalization/vote_recorder -> injector -> proposer/multiwallet -> wallet/wallet -> esperanza/checks
< Circular dependency: esperanza/checks -> finalization/vote_recorder -> injector -> staking/transactionpicker -> miner -> esperanza/checks
21a15
> Circular dependency: esperanza/finalizationstate -> validation -> finalization/vote_recorder -> esperanza/finalizationstate
22a17
> Circular dependency: esperanza/vote -> keystore -> script/sign -> policy/policy -> validation -> finalization/vote_recorder -> esperanza/vote
23a19
> Circular dependency: finalization/state_db -> validation -> injector -> finalization/state_db
25,26c21
< Circular dependency: finalization/state_processor -> snapshot/creator -> snapshot/p2p_processing -> injector -> finalization/state_processor
< Circular dependency: finalization/vote_recorder -> injector -> p2p/finalizer_commits_handler -> p2p/finalizer_commits_handler_impl -> finalization/vote_recorder
---
> Circular dependency: finalization/state_repository -> validation -> injector -> finalization/state_repository
30d24
< Circular dependency: init -> injector -> settings -> init
38c32
< Circular dependency: injector -> p2p/finalizer_commits_handler -> p2p/finalizer_commits_handler_impl -> snapshot/p2p_processing -> injector
---
> Circular dependency: injector -> staking/active_chain -> validation -> injector
41d34
< Circular dependency: injector -> validation -> injector

scravy commented 5 years ago

Irrelevant to this commit, but what do you think if we replace includes with forward declarations in the injector.h? This file becomes a point which triggers recompilation of ~20 .cpp files when only one component is changed or added.

I was thinking about that, tried it, and does not work; the injector needs to have access to the full declaration as it does type resolution doing typeid(...) which does not work with a forward declaration.

scravy commented 5 years ago

With this change walletextention will segfault when configured with no injector. This case must be at least properly asserted in functions who uses this dep. Alternatively we can use stub here.

The wallet extension is a bit of a nightmare, because the wallet is a bit of a nightmare. There is the walletdependencies class to inject stuff into the wallet extension which makes it semi-dependency-injected (it's not managed by the injector yet it's sort of dependency injected as it is created by the wallet using the injector, but there are possibly many instances of a wallet).

The wallet dependencies constructor which does not take the injector is not meant to be used in production. I will add a comment to it. It is there purely because CWallet has a mode (which is for tests only) in which you can create it without a wallet db (without any dependencies at all). This only happens in the unit tests, none of which segfault.

I will add a comment and an assert.

scravy commented 5 years ago

@frolosofsky I hope I can alleviate your concerns with the changes in b154fc347165dc3bf5c2241b77691b567b590501

Rebased, fixed conflicts, force pushed.

scravy commented 5 years ago

I wonder why PR that lands new RPCs, is surrounded by tons of code style refactoring instead of functional tests for new RPC commands.

I haven't check tracestake in details as it is quite complicated (I don't say it must be rewritten or something, just a fact). I will play with this closer locally later.

Some of these changes are because I'm polishing RPC all together. Thank you for bearing with me and doing the effort of reviewing it nevertheless. It should be the same as a bunch of smaller PRs ;-)

scravy commented 5 years ago

Please extract src/rpc/net.cpp code style changes to another PR. It's alsmost impossible to review actual changes in that file.

@frolosofsky That file was accidentally formatted and committed. Undone. I removed it form the commits and force-pushed.

scravy commented 5 years ago

We discussed this a bit and decided to merge despite the concerns. I for one do share these concerns, but I do have to admit that at the same time I'm torn:

The changes do not affect an existing feature (there are a few changes which slipped through but they were reviewed and are covered by tests)
The clean-up of RPC does make sense as a whole
Regarding functional tests: These calls are precisely there to aid writing functional tests and I will follow-up with some shortly, there purpose being validating the stake chain is setup correctly

Action Items:

I will follow-up with a pull request on :: in injector.h where we can continue that discussion, which is the only unresolved item here. I think any conclusion should go to the developer guide and I am leaning in favor of removing :: altogether (in my mind classes, even though in different namespaces but in the same project, should have different names anyway or be used within the namespace only, but that's a different issue/story which is mostly just a personal preference)
- update: here is said pull request: https://github.com/dtr-org/unit-e/pull/918
I am working on a pull request right now which I'll send shortly which checks stake and comes with a bunch of functional tests (but it needs to be integrated with #808 and this).

As this PR has a couple of approvals and no actual request for changes I hope this is sort of okay. We need to integrate #808, this, that upcoming pull request and it is saving a lot of time and effort to do it via master.

In any case thanks for the good reviews, I think the code is now much more aligned and less alien. There's a couple of things, also refactorings (when already touched then also do), which I'll push in another pull request, which I guess is what we lean to anyway.

dtr-org / unit-e

RPC methods tracechain, tracestake, getstakeablecoins #900

`tracechain`

`tracestake`

`getstakeablecoins`

How to test / How to use

`getstakeablecoins`

`tracestake`