As orphan state is now "network state", like in
d6ea737be1,
UnloadBlockIndex is only used during init if we end up reindexing
to clear our block state so that we can start over. However, at
that time no connections have been brought up as CConnman hasn't
been started yet, so all of the network processing state logic is
empty when its called.
This fixes one of the last major layer violations in the networking stack.
The network side is no longer in charge of message serialization, so it is now
decoupled from Bitcoin structures. Only the header is serialized and attached
to the payload.
Also, send reject messages earlier in SendMessages(), so that disconnections are
processed earlier.
These changes combined should ensure that no message is ever sent after
fDisconnect is set.
This way we're not relying on messages going out after fDisconnect has been
set.
This should not cause any real behavioral changes, though feelers should
arguably disconnect earlier in the process. That can be addressed in a later
functional change.
When a BIP152 HB-mode peer is in the least preferred position and
disconnects, they will not be by ForNode on the next loop. They
will continue to sit in that position and prevent deactivating
HB mode for peers that are still connected.
There is no reason for them to stay in the list if already gone,
so drop the first element unconditionally if there are too many.
Fixes issue #9163.
In 0.13 orphan transactions began being treated as implicit
INVs for their parents. But the resulting getdata were
not getting the witness flag.
This fixes issue #9182 reported by chjj and roasbeef on IRC.
This further decouples "main" and "net" processing logic by moving
orphan processing out of the chain-connecting cs_main lock and
into its own cs_main lock, beside all of the other chain callbacks.
Once further decoupling of net and main processing logic occurs,
orphan handing should move to its own lock, out of cs_main.
Note that this will introduce a race if there are any cases where
we assume the orphan map to be consistent with the current chain
tip, however I am confident there is no such case (ATMP will fail
without DoS score in all such cases).
Given that in default GetSerializeSize implementations created by
ADD_SERIALIZE_METHODS we're already using CSizeComputer(), get rid
of the specialized GetSerializeSize methods everywhere, and just use
CSizeComputer. This removes a lot of code which isn't actually used
anywhere.
For CCompactSize and CVarInt this actually removes a more efficient
size computing algorithm, which is brought back in a later commit.
The current getblocktxn implementation drops and ignores requests for old
blocks, which causes occasional sync_block timeouts during the
p2p-compactblocks.py test as reported in
https://github.com/bitcoin/bitcoin/issues/8842.
The p2p-compactblocks.py test setup creates many new blocks in a short
period of time, which can lead to getblocktxn requests for blocks below the
hardcoded depth limit of 10 blocks. This commit changes the getblocktxn
handler not to ignore these requests, so the peer nodes in the test setup
will reliably be able to sync.
The protocol change is documented in BIP-152 update "Allow block responses
to getblocktxn requests" at https://github.com/bitcoin/bips/pull/469.
The protocol change is not expected to affect nodes running outside the test
environment, because there shouldn't normally be lots of new blocks being
rapidly added that need to be synced.
The changes here are dense and subtle, but hopefully all is more explicit
than before.
- CConnman is now in charge of sending data rather than the nodes themselves.
This is necessary because many decisions need to be made with all nodes in
mind, and a model that requires the nodes calling up to their manager quickly
turns to spaghetti.
- The per-node-serializer (ssSend) has been replaced with a (quasi-)const
send-version. Since the send version for serialization can only change once
per connection, we now explicitly tag messages with INIT_PROTO_VERSION if
they are sent before the handshake. With this done, there's no need to lock
for access to nSendVersion.
Also, a new stream is used for each message, so there's no need to lock
during the serialization process.
- This takes care of accounting for optimistic sends, so the
nOptimisticBytesWritten hack can be removed.
- -dropmessagestest and -fuzzmessagestest have not been preserved, as I suspect
they haven't been used in years.
This introduces a 'minimum chain work' chainparam which is intended
to be the known amount of work in the chain for the network at the
time of software release. If you don't have this much work, you're
not yet caught up.
This is used instead of the count of blocks test from checkpoints.
This criteria is trivial to keep updated as there is no element of
subjectivity, trust, or position dependence to it. It is also a more
reliable metric of sync status than a block count.
This will result in many more calls to CheckBlockIndex when
connecting a list of headers (eg in ::HEADERS messages processing)
but its only enabled in debug mode, and that should mostly just be
during IBD, so it should be OK.
UnloadBlockIndex is only used during init if we end up reindexing
to clear our block state so that we can start over. However, at
that time no connections have been brought up as CConnman hasn't
been started yet, so all of the network processing state logic is
empty when its called.
Additionally, the initialization of the recentRejects set is moved
to InitPeerLogic.
This change is needed to prevent sync_blocks timeouts in the mempool_reorg
test after the sync_blocks update in the upcoming commit
"[qa] Change sync_blocks to pick smarter maxheight".
This change was initially suggested by Suhas Daftuar <sdaftuar@chaincode.com>
in https://github.com/bitcoin/bitcoin/pull/8680#r78209060
Pieter Wuille