When block is being spread through the network we can get a lot of invs with
the same hash. Some more stale nodes may also announce previous or some
earlier block. We can avoid full DB lookup for them and minimize inv handling
time (timeouts in inv handler had happened in #2744).
It doesn't affect tests, just makes node a little less likely to spend some
considerable amount of time in the inv handler.
Sometimes we already have it, but it's not yet processed, so we can save on
getdata request. It only affects very high-speed networks like 4-1 scenario
and it doesn't affect it a lot, but still we can do it.
This is not exactly the protocol-level batching as was tried in #1770 and
proposed by neo-project/neo#2365, but it's a TCP-level change in that we now
Write() a set of messages and given that Go sets up TCP sockets with
TCP_NODELAY by default this is a substantial change, we have less packets
generated with the same amount of data. It doesn't change anything on properly
connected networks, but the ones with delays benefit from it a lot.
This also improves queueing because we no longer generate 32 messages to
deliver on transaction's GetData, it's just one stream of bytes with 32
messages inside.
Do the same with GetBlocksByIndex, we can have a lot of messages there too.
But don't forget about potential peer DoS attacks, if a peer is to request a
lot of big blocks we need to flush them before we process the whole set.
This allows to naturally scale transaction processing if we have some peer
that is sending a lot of them while others are mostly silent. It also can help
somewhat in the event we have 50 peers that all send transactions. 4+1
scenario benefits a lot from it, while 7+2 slows down a little. Delayed
scenarios don't care.
Surprisingly, this also makes disconnects (#2744) much more rare, 4-node
scenario almost never sees it now. Most probably this is the case where peers
affect each other a lot, single-threaded transaction receiver can be slow
enough to trigger some timeout in getdata handler of its peer (because it
tries to push a number of replies).
It makes sense in general (further narrowing down the time window when
transactions are processed by consensus thread) and it improves block times a
little too, especially in the 7+2 scenario.
Related to #2744.
Until the consensus process starts for a new block and until it really needs
some transactions we can spare some cycles by not delivering transactions to
it. In tests this doesn't affect TPS, but makes block delays a bit more
stable. Related to #2744, I think it also may cause timeouts during
transaction processing (waiting on the consensus process channel while it does
something dBFT-related).
When the network is big enough, MinPeers may be suboptimal for good network
connectivity, but if we know the network size we can do some estimation on the
number of sufficient peers.
Share parameters parsing code between 'contract invokefunction' and
'vm run' commands. It allows VM CLI to parse more complicated parameter
types including arrays and file-backed bytestrings.
They can fail right in the getPeers or they can fail later when packet send
is attempted. Of course they can complete handshake in-between these events,
but most likely they won't and we'll waste more resources on this attempt. So
rule out bad peers immediately.
Drop EnqueueP2PPacket, replace EnqueueHPPacket with EnqueueHPMessage. We use
Enqueue* when we have a specific per-peer message, it makes zero sense
duplicating serialization code for it (unlike Broadcast*).
Follow the general rules of broadcasts, even though it's somewhat different
from Inv, we just want to get some reply from our neighbors to see if we're
behind. We don't strictly need all neighbors for it.
We have a number of queues for different purposes:
* regular broadcast queue
* direct p2p queue
* high-priority queue
And two basic egress scenarios:
* direct p2p messages (replies to requests in Server's handle* methods)
* broadcasted messages
Low priority broadcasted messages:
* transaction inventories
* block inventories
* notary inventories
* non-consensus extensibles
High-priority broadcasted messages:
* consensus extensibles
* getdata transaction requests from consensus process
* getaddr requests
P2P messages are a bit more complicated, most of the time they use p2p queue,
but extensible message requests/replies use HP queue.
Server's handle* code is run from Peer's handleIncoming, every peer has this
thread that handles incoming messages. When working with the peer it's
important to reply to requests and blocking this thread until we send (queue)
a reply is fine, if the peer is slow we just won't get anything new from
it. The queue used is irrelevant wrt this issue.
Broadcasted messages are radically different, we want them to be delivered to
many peers, but we don't care about specific ones. If it's delivered to 2/3 of
the peers we're fine, if it's delivered to more of them --- it's not an
issue. But doing this fairly is not an easy thing, current code tries performing
unblocked sends and if this doesn't yield enough results it then blocks (but
has a timeout, we can't wait indefinitely). But it does so in sequential
manner, once the peer is chosen the code will wait for it (and only it) until
timeout happens.
What can be done instead is an attempt to push the message to all of the peers
simultaneously (or close to that). If they all deliver --- OK, if some block
and wait then we can wait until _any_ of them pushes the message through (or
global timeout happens, we still can't wait forever). If we have enough
deliveries then we can cancel pending ones and it's again not an error if
these canceled threads still do their job.
This makes the system more dynamic and adds some substantial processing
overhead, but it's a networking code, any of this overhead is much lower than
the actual packet delivery time. It also allows to spread the load more
fairly, if there is any spare queue it'll get the packet and release the
broadcaster. On the next broadcast iteration another peer is more likely to be
chosen just because it didn't get a message previously (and had some time to
deliver already queued messages).
It works perfectly in tests, with optimal networking conditions we have much
better block times and TPS increases by 5-25%% depending on the scenario.
I'd go as far as to say that it fixes the original problem of #2678, because
in this particular scenario we have empty queues in ~100% of the cases and
this new logic will likely lead to 100% fan out in this case (cancelation just
won't happen fast enough). But when the load grows and there is some waiting
in the queue it will optimize out the slowest links.
Value of PublicKey parameter always stores public key bytes, not the
deserialized representation. All other code (CLI parameters parsing with
its NewParameterFromString, Parameter unmarshaller, etc.) is based on
the idea that value of PublicKey is []byte.
Peers can be slow, very slow, slow enough to affect node's regular
operation. We can't wait for them indefinitely, there has to be a timeout for
send operations.
This patch uses TimePerBlock as a reference for its timeout. It's relatively
big and it doesn't affect tests much, 4+1 scenarios tend to perform a little
worse with while 7+2 scenarios work a little better. The difference is in some
percents, but all of these tests easily have 10-15% variations from run to
run.
It's an important step in making our gossip better because we can't have any
behavior where neighbors directly block the node forever, refs. #2678 and
Refs. #2379, but not completely solves it, one package seriously outweights
others:
? github.com/nspcc-dev/neo-go/cli [no test files]
ok github.com/nspcc-dev/neo-go/cli/app 0.036s coverage: 100.0% of statements
ok github.com/nspcc-dev/neo-go/cli/cmdargs 0.011s coverage: 60.8% of statements
ok github.com/nspcc-dev/neo-go/cli/flags 0.009s coverage: 97.7% of statements
? github.com/nspcc-dev/neo-go/cli/input [no test files]
ok github.com/nspcc-dev/neo-go/cli/options 0.033s coverage: 50.0% of statements
? github.com/nspcc-dev/neo-go/cli/paramcontext [no test files]
ok github.com/nspcc-dev/neo-go/cli/query 2.155s coverage: 45.3% of statements
ok github.com/nspcc-dev/neo-go/cli/server 1.373s coverage: 67.8% of statements
ok github.com/nspcc-dev/neo-go/cli/smartcontract 8.819s coverage: 94.3% of statements
ok github.com/nspcc-dev/neo-go/cli/util 0.006s coverage: 10.9% of statements
? github.com/nspcc-dev/neo-go/cli/vm [no test files]
ok github.com/nspcc-dev/neo-go/cli/wallet 72.103s coverage: 88.2% of statements
Still a nice thing to have.
In case of ellipsis usage compiler defines argument type as ArrayT
(which is correct, because it's a natural representation of the last
argument, it represents the array of interface{}).
Here goes the problem:
```
=== RUN TestEventWarnings/variadic_event_args_via_ellipsis
compiler_test.go:251:
Error Trace: compiler_test.go:251
Error: Received unexpected error:
event 'Event' should have 'Integer' as type of 1 parameter, got: Array
Test: TestEventWarnings/variadic_event_args_via_ellipsis
```
Parsing the last argument in this case is a separate complicated problem
due to the fact that we need to grab types of elements of []interface{} inside the
fully qualified ast node which may looks like:
```
runtime.Notify("Event", (append([]interface{}{1, 2}, (([]interface{}{someVar, 4}))...))...)
```
Temporary solution is to exclude such notifications from analysis until we're
able to properly resolve element types of []interface{}.
It's possible that declared manifest event has parameter of AnyT for
those cases when parameter type differs from method to method. If so,
then we don't need to enforce type check after compilation.
Which greatly simplifies reuse of these packages (and they're expected to be
reused since real tokens implement standards and also add something of their
own) and allows to avoid effects like
doc_test.go:68:28: ambiguous selector neoContract.BalanceOf
when neo.Contract is used. Avoids duplication in NEP-11 implementation as
well.
So that (*codegen).Visit is able to omit code generation for these
unused global vars. The most tricky part is to detect unused global
variables, it is done in several steps:
1. Collect the set of named used/unused global vars.
2. Collect the set of globally declared expressions that contain
function calls.
3. Pick up global vars from the set made at step 2.
4. Traverse used functions and puck up those global vars that are used
from these functions.
5. Rename all globals that are presented in the set made at step 1
but are not presented in the set made on step 3 or step 4.
Move all auxiliary function declaration after Main, so that INITSLOT
instructions counter works properly. `vmAndCompileInterop` loads program
and moves nextIP to the Main function offset if there's no _init
function. If _init is there, then nextIP will be moved to the start of
_init. In TestInline we don't handle instructions properly (CALL/JMP
don't change nextIP), we just perform instruction traversal from the
start point via Next(), thus INITSLOT counter value depends on the
starting instruction, which depends on _init presence.
If variable is unnamed and does not contain function call then it's
treated as unused and code generation may be omitted for it
initialization/declaration.
In case if global var is unnamed (and, as a consequence, unused) and
contains a function call inside its value specification, we need to emit
code for this var to be able to call the function as it can have
side-effects. See the example:
```
package foo
import "github.com/nspcc-dev/neo-go/pkg/interop/runtime"
var A = f()
func Main() int {
return 3
}
func f() int {
runtime.Notify("Valuable notification", 1)
return 2
}
```
NEP-6 has a notion of locked acccounts and SignTx must respect this user's
choice. For some reason this setting was inappropriately used by our RPC
client tests (probably a different kind of lock was meant).
* each account must have an appropriate signer, if there is no signer for
this account in the tx it's an error
* we can only safely append to Scripts when account belongs to the next
signer (we don't have appropriate verification scripts for other signers)
* when contract has one parameter, the signature shouldn't be appended to
other data
I think these rules allow to handle more cases and do that safer. We have more
complex scenarios though, like non-signature parameters or mixed-parameter
invocation scripts, but that's out of scope for now.
