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

rpc, cli: support gas claim and asset transfer #694

Merged
merged 22 commits into from
Mar 2, 2020

Conversation

fyrchik
Copy link
Contributor

@fyrchik fyrchik commented Feb 26, 2020

Closes #669, #670 .

Also:

  1. Calculate amount of GAS available for claiming.
  2. Add contractTX to the test chain.
  3. Track spent coins for every account.
  4. Add CLI to claim GAS

@codecov
Copy link

codecov bot commented Feb 26, 2020

Codecov Report

Merging #694 into master will decrease coverage by 0.33%.
The diff coverage is 44.88%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #694      +/-   ##
==========================================
- Coverage   65.76%   65.42%   -0.34%     
==========================================
  Files         128      128              
  Lines       10916    11084     +168     
==========================================
+ Hits         7179     7252      +73     
- Misses       3456     3550      +94     
- Partials      281      282       +1
Impacted Files Coverage Δ
pkg/rpc/server/prometheus.go 100% <ø> (ø) ⬆️
pkg/core/spent_coin_state.go 100% <ø> (ø) ⬆️
pkg/core/state/account.go 65.67% <13.33%> (-15.1%) ⬇️
pkg/core/blockchain.go 30.24% <32.2%> (+0.12%) ⬆️
pkg/core/dao.go 66.66% <83.33%> (+0.1%) ⬆️
pkg/rpc/server/server.go 84.54% <91.89%> (+0.63%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7d59fa0...29484ed. Read the comment docs.

pkg/core/helper_test.go Outdated Show resolved Hide resolved
pkg/core/helper_test.go Show resolved Hide resolved
pkg/core/blockchain.go Show resolved Hide resolved
pkg/core/blockchain.go Outdated Show resolved Hide resolved
pkg/core/blockchain.go Show resolved Hide resolved
pkg/core/blockchain.go Show resolved Hide resolved
pkg/core/state/account.go Show resolved Hide resolved
pkg/rpc/server/server_helper_test.go Outdated Show resolved Hide resolved
pkg/core/blockchain.go Outdated Show resolved Hide resolved
pkg/core/blockchain.go Show resolved Hide resolved
pkg/core/spent_coin_state.go Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Feb 27, 2020

Codecov Report

Merging #694 into master will decrease coverage by 0.32%.
The diff coverage is 40%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #694      +/-   ##
==========================================
- Coverage   65.94%   65.61%   -0.33%     
==========================================
  Files         130      130              
  Lines       11196    11381     +185     
==========================================
+ Hits         7383     7468      +85     
- Misses       3525     3621      +96     
- Partials      288      292       +4
Impacted Files Coverage Δ
pkg/rpc/server/prometheus.go 100% <ø> (ø) ⬆️
pkg/core/spent_coin_state.go 100% <ø> (ø) ⬆️
pkg/rpc/request/txBuilder.go 51.36% <0%> (+5.63%) ⬆️
pkg/wallet/account.go 80.76% <0%> (-12.57%) ⬇️
pkg/core/transaction/transaction.go 58.25% <10%> (-5.19%) ⬇️
pkg/smartcontract/param_type.go 91.61% <100%> (ø) ⬆️
pkg/vm/stack_item.go 80.86% <100%> (ø) ⬆️
pkg/core/state/account.go 65.67% <13.33%> (-15.1%) ⬇️
pkg/core/blockchain.go 31.03% <31.2%> (+0.44%) ⬆️
pkg/consensus/consensus.go 41.06% <62.5%> (+0.36%) ⬆️
... and 4 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 252a9f2...1264b62. Read the comment docs.

@codecov
Copy link

codecov bot commented Feb 27, 2020

Codecov Report

Merging #694 into master will decrease coverage by 0.35%.
The diff coverage is 44.13%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #694      +/-   ##
=========================================
- Coverage   65.76%   65.4%   -0.36%     
=========================================
  Files         128     128              
  Lines       10916   11087     +171     
=========================================
+ Hits         7179    7252      +73     
- Misses       3456    3553      +97     
- Partials      281     282       +1
Impacted Files Coverage Δ
pkg/rpc/server/prometheus.go 100% <ø> (ø) ⬆️
pkg/core/spent_coin_state.go 100% <ø> (ø) ⬆️
pkg/core/state/account.go 65.67% <13.33%> (-15.1%) ⬇️
pkg/core/blockchain.go 30.16% <31.4%> (+0.04%) ⬆️
pkg/core/dao.go 66.66% <83.33%> (+0.1%) ⬆️
pkg/rpc/server/server.go 84.54% <91.89%> (+0.63%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7d59fa0...b1a6a49. Read the comment docs.

@roman-khimov roman-khimov added this to the v0.74.0 milestone Feb 28, 2020
pkg/core/blockchain.go Show resolved Hide resolved
cli/wallet/wallet.go Outdated Show resolved Hide resolved
pkg/core/dao.go Outdated Show resolved Hide resolved
// TODO: storeBlock needs some more love, its implemented as in the original
// project. This for the sake of development speed and understanding of what
// is happening here, quite allot as you can see :). If things are wired together
// and all tests are in place, we can make a more optimized and cleaner implementation.
func (bc *Blockchain) storeBlock(block *block.Block) error {
cache := newCachedDao(bc.dao.store)
if err := cache.StoreAsBlock(block, 0); err != nil {
fee := bc.getSystemFeeAmount(block.PrevHash)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have topBlock in Blockchain, maybe it's worth saving sysFee the same way?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

topBlock is used to get top block/header without accessing the database.
With sysFee I see no situations where it can be useful. sysFee is needed in claim calculations and all blocks accessed there are surely not from the top.
With possible auto-claiming in NEO3.0 it can be useful though.

pkg/core/blockchain.go Outdated Show resolved Hide resolved
cli/wallet/wallet.go Outdated Show resolved Hide resolved
pkg/core/transaction/transaction.go Outdated Show resolved Hide resolved
@fyrchik
Copy link
Contributor Author

fyrchik commented Feb 29, 2020

Regarding sometimes failing tests:
It can be reproduced more surely if t.Parallel() is added in every t.Run() in TestUnexpectedNonInterops. And this happens on master too, so I think it is most likely not a problem of this PR.
I is probably related to a global newBlockPrevHash in tests.

Regarding GolangCI: it thinks getInvocationScript is unused because it is used in a function named _.

Calculating amount of GAS that can be claimed is required
for getclaimable RPC.
To make it easy to get unclaimed coins for the
specified account they must be tracked together.
Marshalling it and taking all but last byte violates incapsulation
and is just wrong in case transaction already contains any witnesses.
Simple transfer from multisig account to the account
of one of the validators.
Write uint32 length before every block.
This is needed to form correct Claim transaction.
When testing CLI it is useful to have some spent coins
on an account with a known key.
GAS can be claimed via `wallet claim` command.
This will claim first get all claimable outputs via
`getclaimable` RPC and then form a transaction signed
byte the private key from the wallet.
Recalculating system fee can be rather costly if done
frequently.
Because accumulated system fee is stored for every block,
it is easy to calculate sum with just to reads.
It uses getunspents RPC for getting UTXO and
forming transaction.
Target of the transaction output may not yet exist in database.
Claim tx have no GAS inputs and a positive output which
can lead to negative network fee.
Otherwise it is possible to make outputs which will sum
to the expected value, but steal GAS from some other account.
It is used in both CLI and RPC.
@roman-khimov roman-khimov merged commit 657f5e4 into master Mar 2, 2020
@roman-khimov roman-khimov deleted the feature/getclaimable branch March 2, 2020 15:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature Completely new functionality rpc RPC server and client
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CLI command to claim gas
3 participants