Every 1000 blocks seems to be OK for big networks (that only had done some
initial requests previously and then effectively never requested addresses
again because there was a sufficient number of addresses), won't hurt smaller
ones as well (that effectively keep doing this on every connect/disconnect,
peer changes are very rare there, but when they happen we want to have some
quick reaction to these changes).
32 is a very good number, but we all know 42 is a better one. And it can even
be proven by tests with higher peaking TPS values.
You may wonder why is it so good? Because we're using packet-switching
networks mostly and a packet is a packet almost irrespectively of how bit it
is. Yet a packet has some maximum possible size (hi, MTU) and this size most
of the time is 1500 (or a little less than that, hi VPN). Subtract IP header
(20 for IPv4 or 40 for IPv6 not counting options), TCP header (another 20) and
Neo message/payload headers (~8 for this case) and we have just a little more
than 1400 bytes for our dear hashes. Which means that in a single packet most
of the time we can have 42-44 of them, maybe 45. Choosing between these
numbers is not hard then.
We have AttemptConnPeers that is closely related, the more we have there the
bigger the network supposedly is, so it's much better than magic minPoolCount.
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.
