reflect.MethodByName is a rather expensive function especially when
called on hot path. This became obvious during profiling of db restore.
This commit replaces reflection with a cast to an interface.
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.
Which speeds it up at least twofold for a typical 32-bytes write (and that's
for a very naïve test that allocates new BufBinWriter on every iteration):
pkg: github.com/CityOfZion/neo-go/pkg/io
BenchmarkWriteBytes-8 10000000 124 ns/op
BenchmarkWriteBytesOld-8 5000000 251 ns/op
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.
To use opcode definitions you have to import whole vm package that you might
not care about at all. So this moves opcodes to their own package under vm, fixes
and deduplicate related code and moves compiler package up one level.
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.
Only request headers from the other peer if his height is bigger than
ours. Otherwise we routinely ask 0-height newcomers for some random headers
that they know nothing about.
This one is essential for the consensus nodes as otherwise they won't give out
the blocks they generate making their generation almost useless. It also makes
our networking part more complete.
We have a race between reader and writer goroutines for the same connection
that leads to handshake failures when reader is faster to read the incoming
version (and try to reply to it) than writer is to write our own Version:
WARN[0000] peer disconnected addr="172.200.0.4:20334" peerCount=5 reason="invalid handshake: tried to send VersionAck, but didn't send Version yet
Fix it by moving Version sending before the reader loop starts.
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).
Param getters were redone to return errors because otherwise bad FuncParam
values could lead to panic. FuncParam itself might be not the most elegant
solution, but it works good enough for now.
This PR does 3 things:
adds array parameter unmarshalling
extend Param with convenient methods
refactor tests into using tables to make it easier add new tests
(part of #347 solution)
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
Right now message can be written in several Write's so
concurrent calls of writeMsg() can in theory interleave.
This commit fixes it.
Signed-off-by: Evgenii Stratonikov <evgeniy@nspcc.ru>
When encountering already seen stack item we should fail
only if it is a collection. Duplicate Integers or ByteArrays are ok
because they can't lead to recursion.
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.
Otherwise the node might crash in `startProtocol` because of missing Version
field in the peer. And it also keeps the sequence correct, Version MUST be
sent first and ACKs can only follow it.
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.
VM: Use JSON-based tests from neoVM
After the implementation of stack limits nothing is needed for us to pass reference JSON tests :)
The only thing that differs --- we do not compare stack in case of FAULT (which matches NEO 3 behavior).
Also two commits were reverted to match 2.x VM behavior.
Our node didn't respect the MaxPeers setting, fix it with a drop of random
connection when this limit is reached (to give a chance for newcomers to
communicate), but also introduce AttemptConnPeers setting to tune the number
of attempted connections.
This also raises the default MaxPeers for testnet/mainnet to 100, because
neo-go nodes love making friends.
This allows to start handshaking from both client and server (mainnet/testnet
nodes were seen to not care about string ordering for it), but still maintains
some sane checks in the process. It also makes functions thread-safe because
we have two goroutines servicing read and write side of the Peer connection,
so they can clash on access to the struct fields.
Add a test for it also.
There is a difference in interpretation of what a block count is. neo-go nodes
currently respond to this request with the latest block number which is the
same number that neoscan.io shows. However, C# nodes deliberately do add one
to this number when answering to the getblockcount request to account for the
genesis block number 0.
This patch makes us consistent with C# nodes wrt to getblockcount behaviour.
This one enables our RPC to be called from the browser if there is a
need. It's insecure and not standards-compliant, thus this behaviour is
configurable is not enabled by default. It makes our node with this workaround
enabled compatible with neo-mon monitoring.
Originally debugged by @anatoly-bogatyrev in #464.
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).
Make inspect work with avms by default and with go files if told so. In the
end this makes our CLI interface more consistent and usable. Drop useless
CompileAndInspect() compiler method along the way.
Keeping run() as the owner of all maps would mean adding at least three more
channels to keep address getters with thread-safety. But then there also is a
race between requestToWork() and run() which is way harder to solve with
channels because there are lots of possibilities for deadlocks. So rework all
of this with good old mutexes.
While at it, fix `requestCh` handling in the inner select of run, it will waste
one loop to handle it, so we should add one to the `requested`.
Fixes#445.
Wrong bits were used to represent flags which is important for contracts
created via interop. Fixes contracts failing to store things:
WARN[16278] contract invocation failed block=3773025 err="error encountered at instruction 3435 (SYSCALL): failed to invoke syscall: contract c9d870d7857e956d82290d5df19de3133c107815 can't have storage" tx=fa695eea240b7b4dbb6f42ea6335447a764d8b629c40b7812ea3bca16b1f098d
WARN[16278] contract invocation failed block=3773025 err="error encountered at instruction 1279 (SYSCALL): failed to invoke syscall: contract 97210e7c98582151ceb37f9748c9a1d27d9ae6fd can't have storage" tx=0144d84038149fa0cf1f7912f7d5854fa5f3670f5b4217789c1441f9fd52d27b
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.
When performing NEWARRAY on a Struct or NEWSTRUCT on a Array,
underlying slice needs to be copied, because when it's capacity
doesn't matches it's length, underlying storage will be used
for appends even if it is already pointed at by another slice.
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)
golint:
pkg/io/binaryrw_test.go:25:11: should omit type []byte from declaration of var bin; it will be inferred from the right-hand side
pkg/io/binaryrw_test.go:42:11: should omit type []byte from declaration of var bin; it will be inferred from the right-hand side
pkg/io/binaryrw_test.go:118:7: should omit type string from declaration of var str; it will be inferred from the right-hand side
golint suggests:
pkg/network/payload/address.go:48:12: should omit type net.IP from declaration of var netip; it will be inferred from the right-hand side
It's a temporary stub until proper encoding/decoding is implemented. It's
useful for testnet/mainnet connections because without it consensus message
receival leads to peer disconnection.
It's bogus and no other node implementation has anything like that. It fires
up for no good reason in the case when some other node connects to us and it
obviously doesn't use its listening port for it.
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.
It gives access to the internal value's Value() which is essential for interop
functions that need to get something from InteropItems. And it also simplifies
some already existing code along the way.
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.
What started as an attempt to fix#366 ended up being quite substantial refactoring of the Blockchain->Store and Server->Blockchain interactions. As usually, some additional problems were noted and fixed along the way. It also accidentally fixes#410.
In the very specific case when the list of headers received is exactly one
block ahead of the chain of full blocks requestBlocks() failed to generate
request to get the next full block.
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.
For example, at the moment our node can't handle `consensus` message, so when
it received it before the patch it just crashed because of uninitialized `p`.
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.
It's mostly used for Serializable and in other cases where one needs to
estimate binary-encoded size of the stucture. This also simplifies future
removal of the Size() from Serializable.
The logic here is that we'll have all binary encoding/decoding done via our io
package, which simplifies error handling. This functionality doesn't belong to
util, so it's moved.
This also expands BufBinWriter with Reset() method to fit the needs of core
package.
add close function to storage interface
add common defer function call which will close db connection
remove context as soon as it's not needed anymore
updated unit tests
This one fixes#390 and some connected problems. After this patchset the node reconnects to some other nodes if anything goes wrong and it better senses when something goes wrong. It also fixes some block handling problems based on the testnet connection experience.
...and don't try to connect to the nodes we're already connected to.
Before this change we had a problem of discoverer throwing away good valid
addresses just because they are already known which lead to pool draining over
time (as address reuse was basically forbidden and getaddr may not get enough
new nodes).
Queuing one message is not reliable enough, the peer that gets it can fail to
actually make a request, so make this queue a bit deeper to have a higher
chance of success.
This makes writer side handle errors properly and fixes communication between
reader and writer goroutine to always correctly unregister the peer. This is
especially important for the case where error occurs before handshake
completes as in this case we don't even have goroutine in startProtocol()
running.
In the unlikely event of overlapping hash block written to the DB we might end
up with wrong hash list. That happened to me for some reason when synching
with the testnet leading to the following keys with respective values:
150000 -> 2000 hashes
152000 -> 2000 hashes
153999 -> 2000 hashes
Reading it hashes number 153999 and 154000 got the same values and the chain
couldn't sync correctly.
Same thing done in a2a8981979 for PUSHBYTES,
failing to read the amount of bytes specified should lead to FAULT. Also
makes readUint16() and readUint32() panic as this is the behavior we want in
these cases. Add some tests along the way.
Before:
NEO-GO-VM > loadgo h.go
READY: loaded 16 instructions
NEO-GO-VM > ip
instruction pointer at -1 (PUSH0)
After:
NEO-GO-VM > loadgo h.go
READY: loaded 16 instructions
NEO-GO-VM > ip
instruction pointer at -1 (NOP)
I think NOP is a little less scary.
Current NEO documentation lists them:
https://docs.neo.org/docs/en-us/tooldev/advanced/neo_vm.html
CALL_* instructions were left out because of conflict with golint (but they're
removed in NEO 3.0 anyway, so wasting time on them makes no sense).
Update autogenerated instruction_string.go accordingly.
The code that we have actually implements XTUCK and not TUCK. And it's a bit
broken, so fix it and add some tests. The most interesting one (that required
to touch stack code) is the one when we have 1 element on the stack and are
trying to tell XTUCK to push 2 elements deep.
ANSI X9.62 says that if x or y coordinate are greater than or equal to
curve.Params().P, the conversion should return an error (see ANSI X9.62:2005
Section A.5.8 Step b, which invokes Section A.5.5, which does the check and
rejects when x or y are too big.
See https://github.com/golang/go/issues/20482 for more details.
PublicKey() for PrivateKey now just can't fail and it makes no sense to return
an error from it. There is a lot of associated functionality for which this
also is true, so adjust it accordingly and simplify a lot of code.
Public key is just a point, so use the coordinates obtained previously to
initialize the PublicKey structure without jumping through the hoops of
encoding/decoding.
As NEO uses P256 we can use standard crypto/elliptic library for almost
everything, the only exception being decompression of the Y coordinate. For
some reason the standard library only supports uncompressed format in its
Marshal()/Unmarshal() functions. elliptic.P256() is known to have
constant-time implementation, so it fixes#245 (and the decompression using
big.Int operates on public key, so nobody really cares about that part being
constant-time).
New decompress function is inspired by
https://stackoverflow.com/questions/46283760, even though the previous one
really did the same thing just in a little less obvious way.
It makes no sense to provide an API for throw-away public keys, so obtain it
via a new real keypair generation where appropriate (and that's only needed
for testing).
Golint:
pkg/rpc/rpc.go:15:67: exported method GetBlock returns unexported type *rpc.response, which can be annoying to use
pkg/rpc/rpc.go:82:64: exported method GetRawTransaction returns unexported type *rpc.response, which can be annoying to use
pkg/rpc/rpc.go:97:52: exported method SendRawTransaction returns unexported type *rpc.response, which can be annoying to use
Refs. #213.
pkg/rpc/neoScanBalanceGetter.go:54:56: method parameter assetIdUint should be assetIDUint
pkg/rpc/neoScanBalanceGetter.go:62:3: var assetId should be assetID
pkg/rpc/server_test.go:27:5: var testRpcCases should be testRPCCases
pkg/rpc/txTypes.go:19:3: struct field assetId should be assetID
pkg/rpc/txTypes.go:39:35: interface method parameter assetId should be assetID
pkg/rpc/types.go:115:2: struct field TxId should be TxID
Refs. #213.
pkg/core/transaction/attribute.go:67:14: should omit type uint8 from declaration of var urllen; it will be inferred from the right-hand side
pkg/crypto/keys/publickey.go:184:8: should omit type []byte from declaration of var b; it will be inferred from the right-hand side
pkg/network/payload/version_test.go:15:12: should omit type bool from declaration of var relay; it will be inferred from the right-hand side
Refs. #213.
Golint:
pkg/core/blockchain.go:796:9: if block ends with a return statement, so drop
this else and outdent its block (move short variable declaration to its own
line if necessary)
Refs. #213.
