This was done in https://github.com/neo-project/neo/pull/1693
for native calls. `OnPersist` script still uses `DROP` though
as value is pushed via `CheckReturn` logic for regular calls.
It can't ever happen. We're guaranteed to have a consistent chain of headers
(we're verifying them above, if we're not verifying --- it's not our fault)
that starts at HeaderHeight that was actual when we were asking for it
previously. HeaderHeight can only move forward, so if that happened that would
be filtered out by the condition below and the first one can't happen. Though
to be absolutely sure change the second check to only pass "+1" headers (which
is what we want).
It's used in two places now:
* Blockchain.AddBlock()
This one does transaction duplication check of its own, doing it in
Verify() is just a waste of time. Merkle tree root hash value check is
still relevant though
* Block.DecodeBinary()
We're decoding blocks for the following purposes:
- on restore from dump
The block will be added to the chain via AddBlock() and that will do a
full check of it (if configured to do so)
- on retrieving the block from the DB (DAO)
We trust the DB, if it's gone wild, this check won't really help
- on receiving the block via P2P
It's gonna be put into block queue and then end up in AddBlock() which
will check it
- on receiving the block via RPC (submitblock)
It is to be passed into AddBlock()
- on receiving the block via RPC in a client
That's the only problematic case probably, but RPC client has to trust
the server and it can check for the signature if it really
cares. Or a separate in-client check might be added.
As we can see nothing really requires this verification to be done the way it
is now, AddBlock can just have a Merkle check and DecodeBinary can do fine
without it at all.
It's a no-op and there is nothing we can do about it, header contents could
only be checked against chain state, there is nothing to check for internal
consistency.
NewMerkleTree is a memory hog, we can do better than that:
BenchmarkMerkle/NewMerkleTree-8 13 88434670 ns/op 20828207 B/op 300035 allocs/op
BenchmarkMerkle/CalcMerkleRoot-8 15 69264150 ns/op 0 B/op 0 allocs/op
It's not needed, we're either creating a new one and assigning it 6 lines
above or we're changing already existing big.Int via a pointer, so no update
is needed.
There is nothing requiring us to do so. It also is bad because it allows for
new transaction to replace some already existing one with the same fee
parameters just because it has "better" hash.
But the other thing is that for transactions with equal fees it's always
better for us to append them to the end of the list, instead of inserting them
in the middle, so this change allows to reduce slice item movements and gain
some 6-7% increase for single-node TPS.
Time is not really relevant for us here and we don't use this timestamp in any
way. Yet it occupies 24 bytes and we do two clock_gettime calls to get it.
Replace it with blockStamp which is going to be used in the future for
transaction retransmissions.
It allows to improve single-node TPS by another 3%.
We're constantly checking for transactions there and most of the time this
check is not successful (meaning that the transaction in question is
new). Bloom filter easily reduces the need to search over the DB in 99% of
these cases and gives some 13% increase in single-node TPS.
Now we have VerifyTx() and PoolTx() APIs that either verify transaction in
isolation or verify it against the mempool (either the primary one or the one
given) and then add it there. There is no possibility to check against the
mempool, but not add a transaction to it, but I doubt we really need it.
It allows to remove some duplication between old PoolTx and verifyTx where
they both tried to check transaction against mempool (verifying first and then
adding it). It also saves us utility token balance check because it's done by
the mempool anyway and we no longer need to do that explicitly in verifyTx.
It makes AddBlock() and verifyBlock() transaction's checks more correct,
because previously they could miss that even though sender S has enough
balance to pay for A, B or C, he can't pay for all of them.
Caveats:
* consensus is running concurrently to other processes, so things could
change while verifyBlock() is iterating over transactions, this will be
mitigated in subsequent commits
Improves TPS value for single node by at least 11%.
Fixes#667, fixes#668.
New transactions are added to the chain with blocks. If there is no
transaction X at height N in DAO, it could only be added with block N+1, so
it has to be present there. Therefore we can replace `dao.HasTransaction()`
check with a search through in-block transactions. HasTransaction() is nasty
in that it may add useless load the DB and this code is being run with a big
Blockchain lock held, so we don't want to be delayed here at all.
Improves single-node TPS by ~2%.
The end effect is almost as if `VerifyTransactions: false` was set in the
config, but without actually compromising the guarantees provided by it.
It almost doubles performance for single-mode benchmarks and makes block
processing smoother (more smaller blocks are being produced).
According to manifest, OnPersist.ReturnType is void, so we shouldn't
return anything from it. It's not so important, as we drop this value at
the end of OnPersist invocation.
C# node is quite picky as it expects there to be exactly one value returned,
but our testchain actually adds 4 signatures for multisig cases instead of 3
which makes it technically incompatible with C# node.
We currently can't process events in codegen, so we have to provide
them via .yml config file. Do not delete the rest of the code connected
with conversion of MethodDebugInfo.Event into manifest.Event as we have
issue #1038.
As it's returned sorted now. Fixes state change mismatch for
NextValidators. It also partially reverts
2f8e7e4d33 and significantly changes the test
chain as the fees are no longer being sent to the same account.
When calling external contracts we expect exactly 1 value to be on
stack. For methods returning nothing, `Null` value is pushed, otherwise
it is an error.`
We were checking blocked accounts twice which is obviously excessive. We also
have our accounts sorted, so we can rely on that in CheckPolicy(). It also
doesn't make much sense to check MaxBlockSystemFee in Blockchain code, policy
contract can handle that.
It no longer depends on blockchain state and there can't ever be an error, in
fact we can always iterate over signers, so copying these hashes doesn't make
much sense at all as well as sorting arrays in verifyTxWitnesses (witnesses
order must match signers order).
It's not needed any more with Go 1.13 as we have wrapping/unwrapping in base
packages. All errors.Wrap calls are replaced with fmt.Errorf, some strings are
improved along the way.
In 121c9664b we should take into account isValid flag of
NativePolicy contract while retrieving MaxVerificationGas native
policy value. Otherwise we won't be able to get MaxVerificationGas
after the node was restarted, because this value is not truly
stored along with the other native policy values.
This commit fixes bug with headers verification after the node
restarting with an existing storage:
```
2020-08-03T12:52:56.158+0300 WARN failed processing headers {"error": "vm failed to execute the script with error: error encountered at instruction 0 (PUSHDATA1): gas limit is exceeded", "errorVerbose": "vm failed to execute the script with error: error encountered at instruction 0 (PUSHDATA1): gas limit is exceeded\ngithub.com/nspcc-dev/neo-go/pkg/core.(*Blockchain).verifyHashAgainstScript\n\t/home/neospcc/Documents/GitProjects/nspcc-dev/neo-go/pkg/core/blockchain.go:1454\ngithub.com/nspcc-dev/neo-go/pkg/core.(*Blockchain).verifyHeaderWitnesses\n\t/home/neospcc/Documents/GitProjects/nspcc-dev/neo-go/pkg/core/blockchain.go:1517\ngithub.com/nspcc-dev/neo-go/pkg/core.(*Blockchain).verifyHeader\n\t/home/neospcc/Documents/GitProjects/nspcc-dev/neo-go/pkg/core/blockchain.go:1175\ngithub.com/nspcc-dev/neo-go/pkg/core.(*Blockchain).addHeaders\n\t/home/neospcc/Documents/GitProjects/nspcc-dev/neo-go/pkg/core/blockchain.go:484\ngithub.com/nspcc-dev/neo-go/pkg/core.(*Blockchain).AddHeaders\n\t/home/neospcc/Documents/GitProjects/nspcc-dev/neo-go/pkg/core/blockchain.go:453\ngithub.com/nspcc-dev/neo-go/pkg/network.(*Server).handleHeadersCmd\n\t/home/neospcc/Documents/GitProjects/nspcc-dev/neo-go/pkg/network/server.go:454\nruntime.goexit\n\t/usr/local/go/src/runtime/asm_amd64.s:1373"}
```
We need to compact our in-memory MPT from time to time, otherwise it quickly
fills up all available memory. This raises two obvious quesions --- when to do
that and to what level do that.
As for 'when', I think it's quite easy to use our regular persistence interval
as an anchor (and it also frees up some memory), but we can't do that in the
persistence routine itself because of synchronization issues (adding some
synchronization primitives would add some cost that I'd also like to avoid),
so do it indirectly by comparing persisted and current height in `storeBlock`.
Choosing proper level is another problem, but if we're to roughly estimate one
full branch node to use 1K of memory (usually it's way less than that) then we
can easily store 1K of these nodes and that gives us a depth of 10 for our
trie.
Signed-off-by: Evgenii Stratonikov <evgeniy@nspcc.ru>
This was differing from C# notion of PrevHash. It's not a previous root, but
rather a hash of the previous serialized MPTRoot structure (that is to be
signed by CNs).
Signed-off-by: Evgenii Stratonikov <evgeniy@nspcc.ru>
Because trie size is rather big, it can't be stored in memory.
Thus some form of caching should also be implemented. To avoid
marshaling/unmarshaling of items which are close to root and are used
very frequenly we can save them across the persists.
This commit implements pruning items at the specified depth,
replacing them by hash nodes.
Signed-off-by: Evgenii Stratonikov <evgeniy@nspcc.ru>
Because there is no distinct type field in JSONized nodes, distinction
is made via payload itself, thus all unmarshaling is done via
NodeObject.
Signed-off-by: Evgenii Stratonikov <evgeniy@nspcc.ru>
MPT is a trie with a branching factor = 16, i.e. it consists of sequences in
16-element alphabet.
Signed-off-by: Evgenii Stratonikov <evgeniy@nspcc.ru>
Get 2 contracts in pair which is useful everytime we need to test
syscall with one contract calling the other.
Signed-off-by: Evgenii Stratonikov <evgeniy@nspcc.ru>
Invoke `_initialize` method on every call if present.
In NEO3 there is no entrypoint and methods are invoked by offset,
thus `Main` function is no longer required.
We still have special `Main` method in tests to simplify them.
Allow to invoke methods by offset:
1. Every invoked contract must have manifest.
2. Check arguments count on invocation.
3. Change AppCall to a regular syscall.
4. Add test suite for `System.Contract.Call`.
Disallow costly verification methods. We put this limit in policy
contract as it may be a subject to change in future.
In fact this value also overrides gas limit for header verification.
Close#1202.
When providing public key as a subslice, it still can be
decoded as a valid key, thus interop will not return an error
but rather push `false` on stack. This test is about providing
invalid key, so ensure this via setting invalid prefix.
Part of #1055.
It should have `AllowStates` flag.
Also removed unreachable code: we can't have such situation when
script container is not a transaction in the scope of `CheckWitness`
method because:
1. Blocks have their own implementation of CheckWitness for
internal usage (it's (bc *Blockchain) verifyHeaderWitnesses method).
2. For the outside calls of System.Runtime.CheckWitness interop (e.g.
calls from smart-contract) script container is always a transaction.
Part of #1055.
Split methods, as they have a lot of common code. This also fixex nil
error of storageGetReadOnlyContext in case when contract does not have
storage.
Fixes#1144. It's quite simple approach, we just update balance info right
upon contract migration. It will slow down migration transactions, but it
takes about 1-2 seconds to Seek through balances at mainnet's 3.8M, so the
approach should still work good enough. The other idea was to make lazy
updates (maintaining contract migration map), but it's more complicated to
implement (and implies that a balance get might also do a write).
There also is a concern about memory usage, it can give a spike of some tens
of megabytes, but that also is considered to be acceptable.
Part of #1055.
We should check contract scripthash against the one provided in manifest
and manifest groups. We shouldn't put on stack anything after return.
And ofcourse, we mast not destroy the old contract at the end, as
`contractDestroy` removes all storage items associated with the
old contract ID (which equals to the new contract ID). We just remove
old contract state - it's enough.
We were accepting transactions with zero system fee, but we shouldn't do
that. Also, transaction's verification execution has to be limited by network
fee.
GetValidators without parameter is called upon DBFT initialization and it
should receive validators for the next block (that will create it),
parameterized GetValidators is used for NextConsensus calculation where we
need a list for the current state of the chain.
NextBlockValidators are updated before the new block persist, so we need to
use GetValidators to get the list corresponding to the current state of the
chain.
Part of #1133
It will help us to use big.Int to store amount of NEP5 tokens. As far as
big.Int doesn't have constant size, we shouldn't use `NEP5TransferSize`
constant anymore.
Even if the value is zero, the GAS distribution updates the balance height, so
storage item must be updated too. Fixes the followin on preview2 testnet:
block 74227: value mismatch for key ffffffff1454a6cb279fbcedc66162ad4ad5d1d910202b92743e000000000000000000000005: 1041032104809fd5002103f3210128010000 vs 1041032104809fd50021033f110128010000
They make no sense. Fixes preview2 testnet state problem:
file BlockStorage_100000/dump-block-70000.json: block 69935: state mismatch for key ffffffff1454a6cb279fbcedc66162ad4ad5d1d910202b92743e000000000000000000000005: Deleted vs Added
And fix contract create to really update the ID, eliminating this difference
in the storage (preview2 testnet):
file BlockStorage_100000/dump-block-39000.json: block 38043: key mismatch: 0c000000617373657464df4ebe92334d1fc7e64b10f1d1e33942d9905e510000000000000009 vs 00000000617373657464df4ebe92334d1fc7e64b10f1d1e33942d9905e510000000000000009
Preview2 testnet:
file BlockStorage_100000/dump-block-12000.json: block 11562: key mismatch: feffffff1454a6cb279fbcedc66162ad4ad5d1d910202b92743e000000000000000000000005 vs feffffff1431b7e7aea5131f74721e002c6a56b610885813f79e000000000000000000000005
Originally this code was written to run after transactions processing, but
after 0fa4c49735 it works in different manner.
ValidatorsCount is not initialized at block 0 with C# node (the first voter
initializes it) and until that initialization happens the standby validators
list is being returned as is without sorting.
Fixes state mismatch for the key ffffffff0e00000000000000000000000000000001 in
the first blocks.
It also affects tests as now the first validator is different and it receives
the network fees.
After block was stored it's possible to have new FeePerByte constraint,
so we should remove all transactions which do not meet this requirement.
Also caching of FeePerByte was added in order not to re-verify
transactions each time mempool needs to be updated.
MarshalJSON should be defined on structure (not pointer), as we use
structures to marshal parameters (e.g. in NotificationEvent and
Invoke of RPC result package) and never use pointers for that purpose.
Also added marshalling of nil array into `[]` instead of `null` to
follow C# implementation.
part of #904
1. We now have MaxTransactionsPerBlock set in native Policy contract,
so this value should be used in (dbft).GetVerified method instead
of passing it as an argument.
2. Removed (dbft).WithTxPerBlock.
2. DBFT API has changed, so update it's version.
3. Removed MaxTransactionsPerBlock from node configuration, as we
have it set in native Policy contract.
C# implementation uses NEWARRAY for creating arguments.
Don't change our implementation in `emit`, because PACK is cheaper and
this script must not depend on the internal details of `emit` package anyway.
There is no such thing as high/low priority transactions, as there are
no free transactions anymore and they are ordered by fees contained
in transaction itself.
Closes#1063.
It's just JSON, io.Serializable is only used for DB storage where the length
should be obtained from the stream. Fixes:
2020-06-18T22:14:10.571+0300 WARN contract invocation failed {"tx": "1ffd475a9c246495d6206cb80a9a78e9d14a433ded60cd37aa87d897655606e1", "block": 25893, "error": "error encountered at instruction 3696 (SYSCALL): failed to invoke syscall: invalid character ':' after top-level value"}
We make it explicit in the appropriate Block/Transaction structures, not via a
singleton as C# node does. I think this approach has a bit more potential and
allows better packages reuse for different purposes.
And implement it for Transaction, the only user of ParameterContext for
now. Which make correct signing/verifying possible for cases when
serialization for general transmission and signing differ.
1. Remove GetScript, IsPayable, GetStorageContext.
2. Revert 82319538 related to GetStorageContext.
3. Rename Migrate to Update.
4. Move remaining to System.Contract.*.
Related #1031.
Updated System.Blockchain.GetBlock interop replaced the functionality of
the following interops:
System.Block.GetTransactions
System.Block.GetTransactionCount
Neo.Block.GetTransactions
Neo.Block.GetTransactionsCount
closes#1025
Now we put on stack stackitem.Array instead of Interop, so we're able to
use all available block properties without extra interop getters.
Removed Neo.Blockchain.GetBlock interop as we don't need it anymore.
Removed Neo.Block.GetTransaction and System.Block.GetTransaction
interops. These interops were replaced by new
System.Blockchain.GetTransactionFromBlock interop.
closes#1023
Now we put on stack stackitem.Array instead of Interop, so we don't
need old transaction-related interops anymore. Removed the following
interops:
System.Transaction.GetHash
Neo.Transaction.GetAttributes
Neo.Transaction.GetHash
Neo.Transaction.GetWitnesses
Neo.Attribute.GetData
Neo.Attribute.GetUsage
Also removed the following duplicated NEO interop:
Neo.Blockchain.GetTransaction
Two changes being done here, because they require a lot of updates to
tests. Now we're back into version 0 and we only have one type of
transaction.
It also removes GetType and GetScript interops, both are obsolete in Neo 3.
When money is being sent usually they go away from someone's pocket, so that
there is a little less money left there. Not in our case as it turns out, we
actually were adding money both to sender and receiver which is nice, but a
bit different from usual economic's expectations.
This syscall should only work for contracts created by current transaction and
that is what is supposed to be checked here. Do so by looking at the
differences between ic.dao and original lower DAO.
Our block.Block was JSONized in a bit different fashion than result.Block in
its NextConsensus and Index fields. It's not good for notifications because
third-party clients would probably expect to see the same format. Also, using
completely different Block representation is probably making our client a bit
weaker as this representation is harder to use with other neo-go components.
So use the same approach we took for Transactions and wrap block.Block which is
to be serialized in proper way.
Fix `Script` JSONization along the way, 3.0 node wraps it within `witnesses`.
Getting batch, updating Prometheus metrics and pushing events doesn't require
any locking: batch is a local cache batch that no one outside cares about,
Prometheus metrics are not critical to be in perfect sync and events are
asynchronous anyway.
Native contracts also don't require any locks and they should be processed
before dumping storage changes.
`NewNEO()` and `NewGAS()` methods are trying to initialise
both `onPersist` and `incBalance` methods of NEO and GAS AFTER
nep5TokenNative is set to the VALUE of created nep5 token.
In this situation an attemmpt to call the corresponding native contracts
methods (e.g. transfer native GAS) leads to contract invocation failure,
as far as `nep5TokenNative.incBalance` method is nil.
Fixed this by initializing both `onPersist` and `incBalance` methods
before getting the value of nep5 contract.
Native contracts deployment creates `Transfer` notifications and adds
them into interop context. However, these notifications were not stored
for two reasons:
1. typo in `Transfer` (so these notifications were not recognised during
processing of the invocation tx in (*Blockchain).storeBlock(...) method)
2. these notifications have `from` adress setted to null, so conversion
to []byte fails. Same thing could happen with `to`.
Related C# issue: https://github.com/neo-project/neo/issues/1646
For now, made both `transfer` and `Transfer` valid.
Rename THROWIFNOT to ASSERT, add ABORT opcode.
ABORT cannot be caught, but the implementation should be postponed until
exception handling is implemented.
1. Dropped `Base.ConsensusData` block field
2. Added `Block.ConsensusData` field with `Nonce` and `PrimaryIndex`
3. Removed "Neo.Header.GetConsensusData" and
"AntShares.Header.GetConsensusData" interops
The notion of NativeContractState shouldn't ever existed, native contract is a
contract and its state is saved as regular contract state which is critical
because we'll have MPT calculations over this state soon.
Initial minting should be done in Neo.Native.Deploy because it generates
notification that should have proper transaction context.
RegisterNative() shouldn't exist as a public method, native contracts are only
registered at block 0 and they can do it internally, no outside user should be
able to mess with it.
Move some structures from `native` package to `interop` also to avoid circular
references as interop.Context has to have a list of native contracts (exposing
them via Blockchainer is again too dangerous, it's too powerful tool).
Our test chain is used in core,rpc and (in future) interop packages.
It is better to have all related declarations in one place to avoid
code duplication.
