If blockchain is not closed, logging in defer can occur
after test has finished, which will lead to a panic with
"Log in goroutine after Test* has completed".
Error in Seek means something is terribly wrong (e.g. db was not opened) and
error drop is not the right thing to do, because caller
will continue working with the wrong view.
Turns out, our dApps use it a lot and we were going to the DB to get it which
is a useless waste of time. Technically we could also remove blockHeight here,
but not doing it at the moment as it's more involved.
It eliminates this time waste from the pprof graph, but doesn't change 1.4M ->
1.5M 100K mainnet block import test case in any noticeable way.
This solves two problems:
* adds support for shortened SYSCALL form that uses IDs (similar to #434, but
for NEO 2.0, supporting both forms), which is important for compatibility
with C# node and mainnet chain that uses it from some height
* reworks interop plugging to use callbacks rather than appending to the map,
these map mangling functions are clearly visible in the VM profiling
statistics and we want spawning a VM to be fast, so it makes sense
optimizing it. This change moves most of the work to the init() phase
making VM setup cheaper.
Caveats:
* InteropNameToID accepts `[]byte` because that's the thing we have in
SYSCALL processing and that's the most often usecase for it, it leads to
some conversions in other places but that's acceptable because those are
either tests or init()
* three getInterop functions are: `getDefaultVMInterop`, `getSystemInterop`
and `getNeoInterop`
Our 100K (1.4M->1.5M) block import time improves by ~4% with this change.
First of all, it was wrong, it was not checking for inputs really, it compared
tx hashes for some reason, second, when it did compare inputs it compared only
the PrevIndex part of them which is also wrong.
Also, there is absolutely no reason to go through GetVerifiedTransactions()
here, we don't need this copy of pointers and it can also be outdated by the
time we're to finish our check.
Before:
BenchmarkTXPerformanceTest-4
5000 485506 ns/op 65886 B/op 409 allocs/op
ok github.com/CityOfZion/neo-go/integration 3.212s
After:
enchmarkTXPerformanceTest-4
5000 371104 ns/op 44367 B/op 408 allocs/op
ok github.com/CityOfZion/neo-go/integration 2.712s
This simple change improves our BenchmarkTXPerformanceTest by 14%, just
because we don't waste time on reallocations during append().
Before:
10000 439754 ns/op 218859 B/op 428 allocs/op
ok github.com/CityOfZion/neo-go/integration 5.423s
After:
10000 369833 ns/op 87209 B/op 412 allocs/op
ok github.com/CityOfZion/neo-go/integration 4.612s
It's useless work being done before it's actually needed. These (updated with
new values) are going to be written with some kind of Put anyway, so writing
them here is just a waste of time.
We're spending a lot of time here, 100K blocks import starting at 1.4M, before
this patch:
real 4m17,748s
user 6m23,316s
sys 0m37,866s
After:
real 3m54,968s
user 5m56,547s
sys 0m39,398s
9% is quite a substantial improvement to justify this change.
Importing 100K blocks starting at 1.4M, before this patch:
real 6m0,356s
user 8m52,293s
sys 0m47,372s
After this patch:
real 4m17,748s
user 6m23,316s
sys 0m37,866s
Almost 30% better.
It's a getter function and even though it's quite fancy with its transactions
processing (for consensus operation) it shouldn't ever change the state of the
Blockchain. If we're to change anything here these changes may conflict with
the actual block processing later or may lead to broken state (if transactions
won't be approved for some reason).
go vet is not happy about them:
pkg/io/binaryReader.go:92:21: method ReadByte() byte should have signature ReadByte() (byte, error)
pkg/io/binaryWriter.go:75:21: method WriteByte(u8 byte) should have signature WriteByte(byte) error
This seriously improves the serialization/deserialization performance for
several reasons:
* no time spent in `binary` reflection
* no memory allocations being made on every read/write
* uses fast ReadBytes everywhere it's appropriate
It also makes Fixed8 Serializable just for convenience.
add dao which takes care about all CRUD operations on storage
remove blockchain state since everything is stored on change
remove storage operations from structs(entities)
move structs to entities package
This change (closely related to the neo-project/neo#1321 proposal) speeds up
1.4M mainnet blocks import by 30%. Basically, we're eliminating key decoding
for block's multisignature that has the same keys most of the time.
Things I don't like about this patch:
* yet another parameter for verifyHashAgainstScript()
* vm keys are not copied in/out
But it's rather simple and solves the problem for this particular case, so I
think it's worth it.
It makes very little sense having pointers here, these structures MUST have
some kind of key and this key is not gonna be wandering somewhere on its
own. Fixes a part of #519.
It reduces heap pressure a little for these elements as we don't have to
allocate/free them individually. And they're directly tied to transactions or
block, not being shared or anything like that, so it makes little sense for
them to be pointer-based. It only makes building transactions a little easier,
but that's obviously a minor usecase.
Before this patch on block import we could easily be spending more than 6
seconds out of 30 in Uint256 encoding for UnspentBalance, now it's completely
off the radar.
When 74590551 introduced this code we had no proper caching layer, so there
were these strange fallbacks in the code. fc0031e5 should'd removed them, but
failed to do so, so do it now and fix processing of transactions that touch
storage for the same key (address) in the same block.
Drop wif.GetVerificationScript(), drop
smartcontract.CreateSignatureRedeemScript(), add GetVerificationScript()
directly to the PublicKey and use it everywhere.
This allows easier reuse of opcodes and in some cases allows to eliminate
dependencies on the whole vm package, like in compiler that only needs opcodes
and doesn't care about VM for any other purpose.
And yes, they're opcodes because an instruction is a whole thing with
operands, that's what context.Next() returns.
Commit c80ee952a1 removed temporary store used
to contain changes of the block being processed. It's wrong in that the block
changes should be applied to the database in a single transaction so that
there wouldn't be any intermediate state observed from the outside (which is
possible now). Also, this made changes commiting persist them to the
underlying store effectively making our persist loop a no-op (and not
producing `persist completed` log lines that we love so much).
add processing of validators while block persist;
add validator structure with decoding/encoding;
add validator get from store;
add EnrollmentTX and StateTX processing;
add pubkey decode bytes, unique and contains functions;
Fixes failure to process transaction from the block when it was relayed
initially:
WARN[0788] blockQueue: failed adding block into the blockchain blockHeight=7270 error="transaction 35088916403e5cf2152e16c3bc6e0fba20c955fba38543b9fa5c50a3d3a4ace5 failed to verify: invalid transaction due to conflicts with the memory pool" nextIndex=7271
WARN[0790] blockQueue: failed adding block into the blockchain blockHeight=7270 error="transaction 35088916403e5cf2152e16c3bc6e0fba20c955fba38543b9fa5c50a3d3a4ace5 failed to verify: invalid transaction due to conflicts with the memory pool" nextIndex=7271
WARN[0790] blockQueue: failed adding block into the blockchain blockHeight=7270 error="transaction 35088916403e5cf2152e16c3bc6e0fba20c955fba38543b9fa5c50a3d3a4ace5 failed to verify: invalid transaction due to conflicts with the memory pool" nextIndex=7271
If we're to receive some 500 headers (less than `headerBatchCount`) and quit
before receiving more of them we end up with clean `bc.headerList` that will
be inited going backwards to the `targetHash`, but code path doesn't add add
the `targetHash` itself which it should do in this particular case, otherwise
we end with no genesis block hash in the list.
Missing it the following line could fail on subsequent restarts:
currHeaderHeight, currHeaderHash, err := storage.CurrentHeaderHeight(bc.store)
if the node was stopped before any headers had been received.
Extend Blockchainer with one more method to spawn a VM for test runs and use
it to run scripts. Gas consumption is not counted or limited in any way at the
moment (see #424).
NewInvocationTX() returned a version number one transaction that actually
failed to pass that version down to the invocation data which lead to
serialization/deserialization inconsistency.
VM should be responsible for code execution and in case anyone interested in additional logging or errors they could handle them like we do it iin cli.
We're about stored values here, so print those, which avoids blocking in
bc.HeaderHeight() and removes duplication between blockHeight and
persistedHeight. Fixes saving the blockchain on exit (deferred function in
Run() blocked in persist()).
Test modification was required because storeBlocks() doesn't actually save
headers and thus TestGetTransaction started to fail on persist().
If you're to sync less than 2000 headers no batched header key-value is
gonna be written into the DB and init() would panic because
bc.headerList.Len() would return 0. Use genesis block as a target in this
case.
Goreport:
neo-go/pkg/core/contract_state_test.go
Line 21: warning: "Contracto" is a misspelling of "Contraction" (misspell)
Line 64: warning: "Contracto" is a misspelling of "Contraction" (misspell)
neo-go/pkg/core/interop_neo.go
Line 420: warning: "succeedes" is a misspelling of "succeeds" (misspell)
neo-go/pkg/network/discovery.go
Line 118: warning: "succeded" is a misspelling of "succeeded" (misspell)
Line 128: warning: "successfuly" is a misspelling of "successfully" (misspell)
commit methods duplicated putSmthIntoStore functions, but have MemCachedStore
now that can easily substitute for a Batch, especially given that interop
needs something like that for its storage purposes anyway.
This adds the following verifications:
* merkleroot check
* index check
* timestamp check
* witnesses verification
VerifyWitnesses is also renamed to verifyTxWitnesses here to not confuse it
with verifyBlockWitnesse and to hide it from external access (no users at the
moment).
Linter isn't happy with our recent changes:
pkg/core/contract_state.go:109:1: receiver name cs should be consistent with previous receiver name a for ContractState
pkg/core/contract_state.go:114:1: receiver name cs should be consistent with previous receiver name a for ContractState
pkg/core/contract_state.go:119:1: receiver name cs should be consistent with previous receiver name a for ContractState
But actually `a` here most probably is a copy-paste from AssetState methods,
so fit the old code to match the new one.
Enable transaction verification for privnets and tests, testnet can't
successfuly verify block number 316711 with it enabled and mainnet stops at
105829.
We want to get a full block, so it has to have transactions
inside. Unfortunately our tests were used to this wrong behavior and utilized
completely bogus transactions without data that couldn't be persisted, so fix
that also.
PublishTX only had one of these flags, but newer contracts (created via the
interop function) can have more and these flags are aggregated into one field
that uses PropertyState enumeration (it's used to publish contract, so
supposedly it's also a nice choice for contract state storage).
It's used a lot and it looks a lot like MemoryStore, it just needs not to
return errors from Put and Delete, so make it use MemoryStore internally with
adjusted interface.
Make it look more like a real transaction, put/delete things with a single
lock. Make a copy of value in Put also, just for safety purposes, no one knows
how this value slice can be used after the Put.
Using pointers is just plain wrong here, because the batch can be updated with
newer values for the same keys.
Fixes Seek() to use HasPrefix also because this is the intended behavior.
Script can return non-bool results that can still be converted to bool
according to the usual VM rules. Unfortunately Bool() panics if this
conversion fails which is OK for things done in vm.execute(), but certainly
not for VerifyWitnesses(), thus there is a need for TryBool() that will just
return an error in this case.
If the block references two ouputs in some other transaction the code failed
to verify it because of key collision. C# code implements it properly by using
full CoinReference type as a key, so let's do it in a similar fashion.
Claim transactions have different logic in C# node, so we need to
implement it too. It's not the most elegant way to fix it, but let's make it
work first and then refactor if and where needed. Fixes verification of Claim
transactions.
BoltDB doesn't have internal batching mechanism, thus we have a substitute for
it, but this substitute is absolutely identical to MemoryBatch, so it's better
to unify them and import ac5d2f94d3 fix into the
MemoryBatch.
Commit 578ac414d4 was wrong in that it saved
only a part of the block, so depending on how you use blockchain, you may
still see that the block was not really processed properly. To really fix it
this commit introduces intermediate storage layer in form of memStore, which
actually is a MemoryStore that supports full Store API (thus easily fitting
into the existing code) and one extension that allows it to flush its data to
some other Store.
It also changes AddBlock() semantics in that it only accepts now successive
blocks, but when it does it guarantees that they're properly added into the
Blockchain and can be referred to in any way. Pending block queing is now
moved into the server (see 8c0c055ac657813fe3ed10257bce199e9527d5ed).
So the only thing done with persist() now is just a move from memStore to
Store which probably should've always been the case (notice also that
previously headers and some other metadata was written into the Store
bypassing caching/batching mechanism thus leading to some inefficiency).
This one will replace blockCache in Blockchain itself as it can and should be
external from it. The idea is that we only feed successive blocks into the
Blockchain and it only stores valid proper Blockchain and nothing else.
This changes the Blockchain to also return unpersisted (theoretically, verified
in the AddBlock!) blocks and transactions, making Add/Get interfaces
symmetrical. It allows to turn Persist into internal method again and makes it
possible to enable transaction check in GetBlock(), thus fixing #366.
It must copy both the value and the key because they can be reused for other
purposes between Put() and PutBatch(). This actually happens with values in
headers processing, leading to wrong data being written into the DB.
Extend the batch test to check for that.
earlier we had an issue with failing test in #353 and other one #305.
Reworked these test to have in-memory database. This led to multiple
changes: made some functions like Hash and Persist public(otherwise
it's not possible to control state of the blockchain); removed
unit_tests storage package which was used mainly for leveldb in unit
tests.
I see these tests not really good since they look like e2e tests and
as for me should be run in separate step against dockerized env or
in case we want to check rpc handler we might want to rework it in order
to have interface for proper unit tests.
As for me this patchset at least makes as safe with not removing totally
previous tests and at the same time CircleCI will be happy now.
