Set stake_modifier + functional tests for it #974

scravy · 2019-04-12T15:37:45Z

The stake_modifier for a block is saved in CBlockIndex and it was not set before. This pull request changes that.

Updated the functional tests such that the snapshot hash calculation correctly takes stake_modifier into account. Notable change: Instead of passing snapshot_meta.hash it passes snapshot_meta which harbours both hash and stake_modifier.

Signed-off-by: Julian Fleischer [email protected]

castarco · 2019-04-12T15:43:02Z

I think feature_block might be failing as the snapshot hash incorporates the stake modifier and I'm not sure the calculation is correct right now. I am running functional tests right now.

As far as I can tell, the snapshot hashes were correctly computed. At almost every step they are checked, and those assertions pass... although I'll look closer to see if the assertions are not "good enough". 🤦‍♂️ I said nothing.

src/proposer/proposer_state.h

scravy · 2019-04-12T15:39:44Z

src/proposer/proposer_state.h

@@ -16,13 +16,13 @@ struct State {

  Status m_status = Status::NOT_PROPOSING;

-  //! \brief how often the proposer looked for an eligible coin
+  //! \brief how many times the proposer looked for an eligible coin


this is a follow up to some earlier PR, requested by @castarco

scravy · 2019-04-12T15:40:29Z

src/staking/block_validator.cpp

-      uint256 &snapshot_hash_out,      //!< [out] The snapshot hash extracted from the scriptSig
+      const CTransaction &tx,          //!< [in] The transaction to check
+      blockchain::Height *height_out,  //!< [out] The height extracted from the scriptSig
+      uint256 *snapshot_hash_out,      //!< [out] The snapshot hash extracted from the scriptSig


This was requested in an earlier review by @frolosofsky, the google style guide recommend using pointers for out parameters.

There is a new caller to this function which is not interested in the outputs, so I adapted to follow the style guide and am passing nullptr further down the road.

scravy · 2019-04-12T15:41:13Z

src/staking/legacy_validation_interface.cpp

        return state.DoS(100, false, REJECT_INVALID, "bad-txnmrklroot", true, "hashMerkleRoot mismatch");
-
+      }


this was a nit by @castarco in an earlier pull that these curly braces were missing.

note this is not touching bitcoin code but a file introduced by us.

scravy · 2019-04-12T15:41:35Z

src/staking/stake_validator.cpp

-  Dependency<blockchain::Behavior> m_blockchain_behavior;
-  Dependency<ActiveChain> m_active_chain;
+  const Dependency<blockchain::Behavior> m_blockchain_behavior;
+  const Dependency<ActiveChain> m_active_chain;


this is just something I'm trying to add in general, dependencies of components can/should be const.

scravy · 2019-04-12T15:41:52Z

src/staking/staking_rpc.cpp

@@ -61,6 +61,7 @@ class StakingRPCImpl : public StakingRPC {
  static inline void BlockInfo(UniValue &obj, const CBlockIndex &block) {
    obj.pushKV("block_hash", ToUniValue(*block.phashBlock));
    obj.pushKV("block_height", ToUniValue(block.nHeight));
+    obj.pushKV("stake_modifier", ToUniValue(block.stake_modifier));


for asserting on it in functional tests

scravy · 2019-04-12T15:42:50Z

src/validation.cpp

-        LogPrint(BCLog::VALIDATION, "%s: Invalid stake found for block=%s failure=%s\n",
-            __func__, block.GetHash().ToString(), stake_validation_result.errors.ToString());
-        return false;
+    if (pindex->nHeight > 0) {


this piece of code is also executed for the genesis block which

does not have stake (so no stake to check)

has a stake modifier which is 0000 by definition

skipping genesis here

scravy · 2019-04-12T15:43:19Z

src/validation.cpp

+            return false;
+        }
+        const boost::optional<staking::Coin> utxo = utxo_view.GetUTXO(block.GetStakingInput().prevout);
+        if (utxo) {


this is not further checked as CheckStake takes care of that

scravy · 2019-04-12T15:44:33Z

src/validation.cpp

+        UTXOViewAdapter utxo_view(GetComponent<staking::ActiveChain>(), view);
+        auto validator = GetComponent<staking::StakeValidator>();
+        const staking::BlockValidationResult coinbase_validation_result =
+            GetComponent<staking::BlockValidator>()->CheckCoinbaseTransaction(*block.vtx[0]);


The coinbase validation has been added here so we can be sure there are staking inputs and CBlock::GetStakingInput does not abort. CheckStake would anyway complain that there's no stake, but need to get the UTXO for computing the stake modifier here already.

test/functional/feature_staking.py

src/validation.cpp

test/functional/feature_staking.py

AM5800 · 2019-04-12T16:03:46Z

Concept ACK
I lack experience in the topic. But it looks correct to me. Especially I like the test that check that stake_modifier is not zero

scravy · 2019-04-13T16:48:59Z

src/Makefile.am

@@ -369,7 +368,6 @@ libunite_server_a_SOURCES = \
  staking/coin.cpp \
  staking/legacy_validation_interface.cpp \
  staking/network.cpp \
-  staking/proof_of_stake.cpp \


moved from libserver to common

src/Makefile.am

scravy · 2019-04-13T16:49:48Z

src/Makefile.am

@@ -551,6 +549,7 @@ libunite_common_a_SOURCES = \
  snapshot/messages.cpp \
  snapshot/iterator.cpp \
  snapshot/indexer.cpp \
+  staking/proof_of_stake.cpp \


moved to common. linking problem in the win32 build otherwise. relies on Solver from this lib (consensus/standard.cpp)

scravy · 2019-04-13T16:50:14Z

src/proposer/proposer_rpc.cpp

@@ -133,7 +133,7 @@ class ProposerRPCImpl : public ProposerRPC {
    return proposerstatus(request);
  }

-  UniValue getstakeablecoins(const JSONRPCRequest &request) const override {
+  UniValue liststakeablecoins(const JSONRPCRequest &request) const override {


whopps, also renamed getstakeablecoins to liststakeablecoins in this pr. like it should.

scravy · 2019-04-13T16:50:43Z

src/rpc/staking.cpp

@@ -19,6 +19,7 @@
  t.appendCommand(NAME.name, &NAME);

 void RegisterStakingRPCCommands(CRPCTable &t) {
+  STAKING_RPC_COMMAND(calcstakemodifier, "txid", "prev");


added this command as suggested by @kostyantyn in the spirit of calcsnapshothash RPC for functional tests

scravy · 2019-04-13T16:50:58Z

src/snapshot/rpc_processing.cpp

@@ -167,7 +167,7 @@ UniValue deletesnapshot(const JSONRPCRequest &request) {
 }

 UniValue calcsnapshothash(const JSONRPCRequest &request) {
-  if (request.fHelp || request.params.size() < 2) {
+  if (request.fHelp || request.params.size() < 4 || request.params.size() > 5) {


the validation here was not right

request.params.size() < 4 || request.params.size() > 5 -> request.params.size() != 4.

Guys you might want to think about that again...

4 and 5 are in, 5 is an optional argument...

scravy · 2019-04-13T16:51:10Z

src/snapshot/rpc_processing.cpp

@@ -217,8 +217,8 @@ UniValue calcsnapshothash(const JSONRPCRequest &request) {
  stream >> inputs;
  stream >> outputs;

-  uint256 stake_modifier = uint256(ParseHex(request.params[2].get_str()));
-  uint256 chain_work = uint256(ParseHex(request.params[3].get_str()));
+  const uint256 stake_modifier = uint256S(request.params[2].get_str());


why u not use standard functions?!

This actually cost me several hours of debugging. uint256 . ParseHex /= uint256S!!

Both functions are used in RPC, ParseHex is used when sending raw event (tx, block), uint256S is used only for uint256 values. At the time of writing, ParseHex seemed more convenient as we will have the same order of bytes in both cases.
Now, looking retrospectively, I think we should change to uint256S to be consistent with other RPCs.

@kostyantyn maybe you can PR it?

@thothd I will. Will take care of this.

scravy · 2019-04-13T16:53:18Z

src/staking/proof_of_stake.cpp

@@ -100,4 +101,32 @@ std::vector<CPubKey> ExtractBlockSigningKeys(const CBlock &block) {
  return ExtractBlockSigningKeys(coinbase_inputs[1]);
 }

+uint256 ComputeKernelHash(const uint256 &previous_block_stake_modifier,


moved these basic functions to proof_of_stake which itself is now in common. calcstakemodifier relies on that same logic here.

scravy · 2019-04-13T16:57:41Z

test/functional/feature_staking.py

        coins = result['stakeable_coins']
        assert_equal(2, len(coins))
-        assert_equal(set(map(lambda x: x['coin']['amount'], coins)), set([Decimal(10000), Decimal(3.75)]))
+        assert_equal(set(map(lambda x: x['coin']['amount'], coins)), {Decimal(10000), Decimal(3.75)})


python 3.1 introduced set literals

nit: In fact, you can also use set literals (+ comprehensions) for the left side:

{x['coin']['amount'] for x in coins}

Signed-off-by: Julian Fleischer <[email protected]>

scravy · 2019-04-13T18:05:27Z

test/functional/test_framework/blocktools.py

    chain_work = 2 + 2*height
+    if isinstance(stake_prevout_txid, CTransaction):


feature block checks invalid transactions... well here we go.

scravy · 2019-04-13T18:05:56Z

test/functional/feature_ltor.py

@@ -231,7 +232,7 @@ def get_empty_block(self, sync_height):
        node0 = self.nodes[0]

        hashprev = uint256_from_str(unhexlify(node0.getbestblockhash())[::-1])
-        snapshot_hash = SnapshotMeta(node0.gettipsnapshot()).hash
+        snapshot_hash = get_tip_snapshot_meta(node0).hash


scravy · 2019-04-13T18:07:25Z

test/functional/feature_staking.py

-        assert_equal(set(map(lambda x: x['coin']['amount'], coins)), set([Decimal(10000), Decimal(3.75)]))
+        assert_equal(set(map(lambda x: x['coin']['amount'], coins)), {Decimal(10000), Decimal(3.75)})
+
+        self.log.info("Check that chaindata is maintained across restarts")


@kostyantyn This one is just for you ;-)

scravy · 2019-04-13T18:10:25Z

rebased, squashed, + force pushed

castarco

utACK b63f72f

Gnappuraz

utACK b63f72f

scravy · 2019-04-15T08:31:39Z

@castarco I'll follow up on that set-literal foreach comprehension in a follow-up PR :-)

scravy added the consensus label Apr 12, 2019

scravy added this to the 0.1 milestone Apr 12, 2019

scravy requested a review from a team April 12, 2019 15:37

scravy self-assigned this Apr 12, 2019

castarco reviewed Apr 12, 2019

View reviewed changes

src/proposer/proposer_state.h Outdated Show resolved Hide resolved

scravy commented Apr 12, 2019

View reviewed changes

AM5800 reviewed Apr 12, 2019

View reviewed changes

src/validation.cpp Show resolved Hide resolved

kostyantyn changed the title ~~Set stake_modifieri + functional tests for it~~ Set stake_modifier + functional tests for it Apr 12, 2019

castarco reviewed Apr 12, 2019

View reviewed changes

test/functional/feature_staking.py Outdated Show resolved Hide resolved

scravy requested a review from Nizametdinov April 12, 2019 17:30

scravy force-pushed the stake-modifier branch 4 times, most recently from 13234ec to 8add7ce Compare April 13, 2019 15:27

scravy commented Apr 13, 2019

View reviewed changes

src/Makefile.am Show resolved Hide resolved

scravy commented Apr 13, 2019

View reviewed changes

Calculate and store stake_modifier, Fix functional tests

b63f72f

Signed-off-by: Julian Fleischer <[email protected]>

scravy force-pushed the stake-modifier branch from 08256e2 to b63f72f Compare April 13, 2019 18:03

scravy commented Apr 13, 2019

View reviewed changes

castarco approved these changes Apr 15, 2019

View reviewed changes

Gnappuraz approved these changes Apr 15, 2019

View reviewed changes

scravy merged commit 3d1ef06 into dtr-org:master Apr 15, 2019

kostyantyn mentioned this pull request Apr 17, 2019

Update UIPs to proposed & add reference implementation dtr-org/uips#18

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Set stake_modifier + functional tests for it #974

Set stake_modifier + functional tests for it #974

scravy commented Apr 12, 2019 •

edited

Loading

castarco commented Apr 12, 2019 •

edited

Loading

scravy Apr 12, 2019

scravy Apr 12, 2019

scravy Apr 12, 2019

scravy Apr 12, 2019

scravy Apr 12, 2019

scravy Apr 12, 2019

scravy Apr 12, 2019

scravy Apr 12, 2019

AM5800 commented Apr 12, 2019

scravy Apr 13, 2019

scravy Apr 13, 2019

scravy Apr 13, 2019

scravy Apr 13, 2019

scravy Apr 13, 2019

frolosofsky Apr 16, 2019

scravy Apr 16, 2019

scravy Apr 13, 2019

scravy Apr 13, 2019

kostyantyn Apr 16, 2019

thothd Apr 16, 2019

kostyantyn Apr 16, 2019

scravy Apr 13, 2019

scravy Apr 13, 2019

castarco Apr 15, 2019 •

edited

Loading

scravy Apr 13, 2019

scravy Apr 13, 2019

scravy Apr 13, 2019

scravy commented Apr 13, 2019

castarco left a comment

Gnappuraz left a comment

scravy commented Apr 15, 2019

		return state.DoS(100, false, REJECT_INVALID, "bad-txnmrklroot", true, "hashMerkleRoot mismatch");

		}

		chain_work = 2 + 2*height
		if isinstance(stake_prevout_txid, CTransaction):

Set stake_modifier + functional tests for it #974

Set stake_modifier + functional tests for it #974

Conversation

scravy commented Apr 12, 2019 • edited Loading

castarco commented Apr 12, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AM5800 commented Apr 12, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

castarco Apr 15, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scravy commented Apr 13, 2019

castarco left a comment

Choose a reason for hiding this comment

Gnappuraz left a comment

Choose a reason for hiding this comment

scravy commented Apr 15, 2019

scravy commented Apr 12, 2019 •

edited

Loading

castarco commented Apr 12, 2019 •

edited

Loading

castarco Apr 15, 2019 •

edited

Loading