Prior to this commit, the peer was connected, and then the services and
connectivity fields in the CNode object were manually set. Instead, send
p2p `version` and `verack` messages, and have net_processing's internal
logic set the state of the node.
This ensures that the node's internal state is consistent with how it
would be set in the live code.
Prior to this commit, `dummyNode1.nServices` was set to `NODE_NONE`
which was not a problem since `CNode::fClient` and
`CNode::m_limited_node` are default initialised to false. Now that we
are doing the actual version handshake, the values of `fClient` and
`m_limited_node` are set during the handshake and cause the test to fail
if we do not set `dummyNode1.nServices` to a reasonable value
(NODE_NETWORK | NODE_WITNESS).
Delay initializing the TxRelay data structure for a peer until we receive
a version message from that peer. At that point we'll know whether it
will ever relay transactions. We only initialize the m_tx_relay
data structure if:
- this isn't an outbound block-relay-only connection; AND
- fRelay=true OR we're offering NODE_BLOOM to this peer
(NODE_BLOOM means that the peer may turn on tx relay later)
This fully comments all the TxRelay members. The only significant change
is to the comment for m_relay_txs. Previously the comment stated that
one of the purposes of the field was that "We don't relay tx invs before
receiving the peer's version message". However, even without the
m_relay_txs flag, we would not send transactions to the peer before
receiving the `version` message, since SendMessages() returns
immediately if fSuccessfullyConnected is not set to true, which only
happens once a `version` and `verack` message have been received.
Move m_next_send_feefilter and m_fee_filter_sent out of the `TxRelay`
data structure. All of the other members of `TxRelay` are related to
sending transactions _to_ the peer, whereas m_fee_filter_sent and
m_next_send_feefilter are both related to receiving transactions _from_
the peer. A node's tx relay behaviour is not always symmetrical (eg a
blocksonly node will ignore incoming transactions, but may still send
out its own transactions), so it doesn't make sense to group the
feefilter sending data with the TxRelay data in a single structure.
This does not change behaviour, since IsBlockOnlyConn() is always equal
to !peer.m_tx_relay. We still don't send feefilter messages to outbound
block-relay-only peers (tested in p2p_feefilter.py).
The only place that segwit=True is for a block that contains only the
coinbase transaction. Since the witness commitment is optional if none
of the transactions have a witness, we can leave it out. This doesn't
change the test coverage, which is testing p2p compact block logic.
Suggested in https://github.com/bitcoin/bitcoin/pull/20799#discussion_r867782119
All uses of CBlockHeaderAndShortTxIDs in the product code are
constructed with fUseWTXID=true, so remove the parameter.
There is one use of the CBlockHeaderAndShortTxIDs constructor with
fUseWTXID=false in the unit tests. This is used to construct a
CBlockHeaderAndShortTxIDs for a block with only the coinbase
transaction, so setting fUseWTXID to true or false makes no difference.
Suggested in https://github.com/bitcoin/bitcoin/pull/20799#pullrequestreview-963480278
This introduces an early exit in PeerManagerImpl::NewPoWValidBlock() if
segwit has not been activated for the block. This means that we won't cache the
block/compact block for fast relay and won't relay the cmpctblock
immediately to peers that have requested hb compact blocks. This is fine
because any block where segwit is not yet activated is buried deep in
the chain, and so compact block relay will not be effective.
It's ok not to cache the block/compact block for fast relay for the same
reason - the block must be very deeply buried in the block chain.
ProcessBlockAvailability() also won't get called for all nodes. This is
also fine, since that function only updates hashLastUnknownBlock
and pindexBestKnownBlock, and is called early in every SendMessages()
call.
Subsequent commits will remove support for other versions of compact blocks.
Add a test that a received `sendcmpct` message with version = 1 is
ignored.
This is constructed before addrman and connman, and destructed afterwards.
netgroupman does not currently do anything, but will have functionality added in future commits.
Also, remove cs_main guard from m_wtxid_relay_peers and make it atomic.
This should be fine since we don't need m_wtxid_relay_peers to be
synchronized with m_wtxid_relay exactly at all times.
After this change, RelayTransaction no longer requires cs_main.
We'll move the transaction relay data into Peer in subsequent commits,
but the inbound eviction logic needs to know if the peer is relaying
txs and if the peer has loaded a bloom filter.
This is currently redundant information with m_tx_relay->fRelayTxes,
but when m_tx_relay is moved into net_processing, then we'll need these
separate fields in CNode.
This distribution is used for more than just the next inv send, so make
the name more generic.
Also rename to "exponential" to avoid the confusion that this is a
poisson distribution.
-BEGIN VERIFY SCRIPT-
ren() { sed -i "s/\<$1\>/$2/g" $(git grep -l "$1" ./src) ; }
ren PoissonNextSend GetExponentialRand
ren "a poisson timer" "an exponential timer"
-END VERIFY SCRIPT-
CTxMemPool::check() will carry out internal consistency checks 1/n times,
where n is set by the `-checkmempool` configuration option. By default,
mempool consistency checks are disabled entirely on mainnet.
Therefore, this change has no effect on mainnet nodes running with
default configuration. It simply removes the responsibility to trigger
mempool consistency checks from net_processing.
This just calls through to AcceptToMemoryPool() internally, and is currently unused.
Also add a new transaction validation failure reason TX_NO_MEMPOOL to
indicate that there is no mempool.
User-facing error messages should not leak internal implementation
details like function names. Update the MEMPOOL_REJECTED error string
from "Transaction rejected by AcceptToMemoryPool" to the more generic
"Transaction rejected by mempool". Also update the MEMPOOL_ERROR error
message from "AcceptToMemoryPool failed" to the more precise "Mempool
internal error" since this error indicates and internal (e.g.
logic/hardware/etc) failure, and not a transaction rejection.
AcceptToMemoryPool() is called for an invalid coinbase transaction, so
setting bypass_limits to true or false has no impact on the test.
The only way that changing bypass_limits from true to false could change
the result would be to change the outcome to INVALID(TX_MEMPOOL_POLICY).
Since the ATMP call in this test results in INVALID(TX_CONSENSUS) both
before and after this change, there is no change in behavior.
AcceptToMemoryPool() is called for transactions with fees above
minRelayTxFee and with the mempool not full, so setting bypass_limits to
true or false has no impact on the test.
The only way that changing bypass_limits from true to false could change
the result would be to change the outcome to INVALID(TX_MEMPOOL_POLICY).
Since all the ATMP calls in this test result in VALID both before and
after this change, there is no change in behavior.
Previously, Add() would return true if the function created a new
AddressInfo object, even if that object could not be successfully
entered into the new table and was deleted. That would happen if the new
table position was already taken and the existing entry could not be
removed.
Instead, return true if the new AddressInfo object is successfully
entered into the new table. This fixes a bug in the "Added %i addresses"
log, which would not always accurately log how many addresses had been
added.
p2p_addrv2_relay.py and p2p_addr_relay.py need to be updated since they
were incorrectly asserting on the buggy log (assuming that addresses are
added to addrman, when there could in fact be new table position
collisions that prevent some of those address records from being added).
Use a (reference) parameter instead of a data member of
CAddrManDeterministic. This will allow us to make Fill() a free function
in a later commit.
Also remove CAddrManDeterministic.m_fuzzed_data_provider since it's no
longer used.
DecopeAsmap is a pure utility function and doesn't have any
dependencies on addrman, so move it to util/asmap.
Reviewer hint: use:
`git diff --color-moved=dimmed-zebra --color-moved-ws=ignore-all-space`
The m_ prefix indicates that a variable is a data member. Using it as
a parameter name is misleading.
Also update the name of the function from copyStats to CopyStats to
comply with our style guide.
Commit 181a1207 introduced an initialization order bug: CAddrMan's
m_asmap must be set before deserializing peers.dat. Restore that
ordering.
review hint: use
`git diff --color-moved=dimmed-zebra --color-moved-ws=ignore-all-space`
Clear() is now only called from the ctor, so just inline the code into
that function.
The LOCK(cs) can be removed, since there can be no data races in the ctor.
Also move the function definition out of the header and into the cpp file.
Just use unique_ptr<CAddrMan>s and reset the pointer if a frest addrman is required.
Also make CAddrMan::Clear() private to ensure that no call sites are missed.
`bool CAddrDB::Read(CAddrMan& addr, CDataStream& ssPeers)` is _only_
called from the tests, and the call to addr.Clear() only exists so that
a test that Clear() is called passes. Remove that test and the call.
Now that we manage the lifespan of node.addrman, we can just reset
node.addrman to a newly initialized CAddrMan if parsing peers.dat
fails, eliminating the possibility that Clear() leaves some old state
behind.
Addrman serialization/deserialization tests are currently in net_tests.cpp.
Move them to addrman_tests.cpp with the rest of the addrman tests.
Reviewer hint: review using `git diff --color-moved=dimmed-zebra`
Currently addrman consistency checks are a compile time option, and are not
enabled in our CI. It's unlikely anyone is running these consistency checks.
Make them a runtime option instead, where users can enable addrman
consistency checks every n operations (similar to mempool tests). Update
the addrman unit tests to do internal consistency checks every 100
operations (checking on every operations causes the test runtime to
increase by several seconds).
Also assert on a failed addrman consistency check to terminate program
execution.
addrman_tests fail when consistency checks are enabled, since the tests
set the deterministic test addrman's nKey value to zero, which is an
invalid value. Change this so that deterministic addrman's nKey value is
set to 1.
This requires updating a few tests that are using magic values derived
from nKey being set to 0.
Now that m_recent_confirmed_transactions is owned by PeerManagerImpl,
and PeerManagerImpl's lifetime is managed by the node context, we can
just default initialize m_recent_confirmed_transactions during object
initialization. We can also remove the unique_ptr indirection.
Now that recentRejects is owned by PeerManagerImpl, and
PeerManagerImpl's lifetime is managed by the node context, we can just
default initialize recentRejects during object initialization. We can
also remove the unique_ptr indirection.
Instead of deserializing addresses, placing them in the buckets, and
then removing them if they're invalid, check first and don't place in
the buckets if they're invalid.
John Newbery