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.

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

generate/generatetoaddress should use block builder #907

Open
scravy opened this issue Apr 8, 2019 · 1 comment
Open

generate/generatetoaddress should use block builder #907

scravy opened this issue Apr 8, 2019 · 1 comment
Assignees
Labels
Milestone

Comments

@scravy
Copy link
Member

scravy commented Apr 8, 2019

generate and generatetoaddress use a half-baked version of proposing blocks but they should use the proposer logic / block builder components.

The code in question is the following:

CValidationState state;
    const staking::ActiveChain *active_chain = GetComponent<staking::ActiveChain>();
    for (const staking::Coin &coin : stakeable_coins) {
      proposer::EligibleCoin eligible_coin = {
          staking::Coin(active_chain->GetBlockIndex(coin.GetTransactionId()),
              COutPoint(coin.GetTransactionId(), coin.GetOutputIndex()),
              CTxOut(coin.GetAmount(), scriptPubKeyIn)),
          GetRandHash(), //TODO UNIT-E: At the moment is not used, since we still have PoW here
          GetComponent<blockchain::Behavior>()->CalculateBlockReward(nHeight),
          0, //TODO UNIT-E: At the moment is not used, since we still have PoW here
          0, //TODO UNIT-E: At the moment is not used, since we still have PoW here
          0 //TODO UNIT-E: At the moment is not used, since we still have PoW here
      };

This code replaces the spending outpoint (!) with the target pubkey.

Using the components ready made for staking the proposing code should read:

    LOCK(m_chain->GetLock());
    const CBlockIndex *current_tip = m_chain->GetTip();
    if (!current_tip) {
      throw JSONRPCError(RPC_IN_WARMUP, "Genesis block not loaded yet (current active chain does not have a tip).");
    }
    const uint256 snapshot_hash = m_chain->ComputeSnapshotHash();
    const staking::CoinSet coins = staking_wallet.GetStakeableCoins();
    if (coins.empty()) {
      throw JSONRPCError(RPC_INTERNAL_ERROR, "No stakeable coins.");
    }
    staking::TransactionPicker::PickTransactionsParameters parameters{};
    const staking::TransactionPicker::PickTransactionsResult txs = m_transaction_picker->PickTransactions(parameters);
    if (!txs) {
      throw JSONRPCError(RPC_INTERNAL_ERROR, txs.error);
    }
    const CAmount fees = std::accumulate(txs.fees.begin(), txs.fees.end(), CAmount(0));
    boost::optional<EligibleCoin> coin = m_proposer_logic->TryPropose(coins);
    if (!coin) {
      throw JSONRPCError(RPC_INTERNAL_ERROR, "No eligible coin available.");
    }
    std::shared_ptr<const CBlock> block =
        m_block_builder->BuildBlock(*current_tip, snapshot_hash, *coin, coins, txs.transactions, fees, staking_wallet);
    if (!block) {
      throw JSONRPCError(RPC_INTERNAL_ERROR, "Failed to build block.");
    }
    if (!ProcessNewBlock(Params(), block, true, nullptr)) {
      throw JSONRPCError(RPC_INTERNAL_ERROR, "ProcessNewBlock: block not accepted.");
    }
@scravy scravy added consensus rpc tests Automated tests labels Apr 8, 2019
@scravy scravy added this to the 0.1 milestone Apr 8, 2019
@scravy scravy self-assigned this Apr 8, 2019
@scravy
Copy link
Member Author

scravy commented Apr 9, 2019

Update: generate is correct in that it does create the right reference, but still there is something fishy. The logic proposed in the second code block here still is the logic which should go here. I had played around with passing the reward address and the stake return address separately to control this in #886 which broke stuff, which should not happen if both were doing the same thing (which apparently they are not).

Anyways, postponing as generate/generatetoaddress are good enough for creating stake validation (All I need for that is the RPCs from #900 to properly assert on the staking state).

@scravy scravy changed the title generate/generatetoaddress are incorrect generate/generatetoaddress should use block builder Apr 10, 2019
@scravy scravy mentioned this issue Apr 10, 2019
@thothd thothd modified the milestones: 0.1, 0.2 Apr 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants