Add ProcessOrphans (move-only) #12935
Conversation
src/net_processing.cpp
Outdated
There was a problem hiding this comment.
Looks like lRemovedTxn still needs to be cleared in this method, unless moving it wholly into ProcessOrphans is appropriate.
There was a problem hiding this comment.
Sorry, that indeed looks ugly right now. I will rebase this on #11775, which removes lRemovedTxn in whole.
|
Concept ACK, both for the addition of testing and due to the fact that ProcessMessage is >1300 lines at the moment. Would be great if other applications of the method were ready, to help inform its interface. |
|
This is a move-only commit - so maybe not the right place - but it might be nice to separate the misbehaving peer punishment logic from the re-attempting to accept orphans to the mempool logic - good first step though |
Because many listeners (eg wallet) will want both types of events to be well-ordered, they are parallel and connected on the backend. However, they are exposed separately to clients to separate the logic (and because, hopefully, eventually, they can be exposed to external clients of Bitcoin Core via libconsensus or similar).
This removes the whole ConnectTrace object, which may make it slightly harder to remove the unbounded-memory-during-reorg bug by throwing blocks out of memory and re-loading them from disk later. Comments are added to validationinterface to note where this should likely happen instead of ConnectTrace.
test/functional/p2p_invalid_tx.py
Outdated
There was a problem hiding this comment.
Seems like it woud be more robust (prevent hangs if the node wasn't disconnected) to call network_thread_join after disconnect_p2ps. This is the pattern used most other places (except feature_block for some reason). This would also make shutdown order the reverse of initialization order, which is a good pattern in general.
There was a problem hiding this comment.
Agree. Should be fixed in both places.
test/functional/p2p_invalid_tx.py
Outdated
There was a problem hiding this comment.
It seems not great to immediately start, stop, and restart the network thread. Could just split up the reconnect method up into connect / disconnect methods to avoid this.
test/functional/p2p_invalid_tx.py
Outdated
There was a problem hiding this comment.
Maybe just take a num_connections=1 optional argument and shorten to
for _ in range(num_connections):
self.nodes[0].add_p2p_connection(P2PDataStore())
test/functional/p2p_invalid_tx.py
Outdated
There was a problem hiding this comment.
Check for reject reason below is now dropped (I guess is because the mininode is no longer whitelisted), so might be clearer to update this comment.
There was a problem hiding this comment.
Can the expect_disconnect case actually wait and verify the disconnect happened?
test/functional/p2p_invalid_tx.py
Outdated
There was a problem hiding this comment.
More test description would be helpful here (it took me a while to figure it out): Test confirms that mininode 1 doesn't get disconnected right away after sending the node an invalid orphan transaction. It then confirms mininode 1 is disconnected later when mininode 0 provides the missing parent which makes it possible to detect the orphan was invalid.
At least this explains the tx_withhold and tx_orphan_2_invalid parts of the new test. I still don't know why tx_orphan_1, tx_orphan_2_no_fee, and tx_orphan_2_valid are added and what they are testing for.
test/functional/p2p_invalid_tx.py
Outdated
There was a problem hiding this comment.
What is "may be rejected by policy" referring to and which transactions does it apply to?
test/functional/p2p_invalid_tx.py
Outdated
There was a problem hiding this comment.
Would be nice just for clarity if this could explicitly verify mininode 1 was disconnected and mininode is still connected. I understand the condition is implied from the peer count combined with expect_disconnect=False above, but it's not clear why this can't just be tested directly.
test/functional/p2p_invalid_tx.py
Outdated
There was a problem hiding this comment.
Could make sense to rebase the test commit before the refactoring commit, to make it clear existing code is supposed to pass the new test.
Or maybe just put the new test in a standalone PR if the refactoring is going to be held up by #11775 anyway (#12935 (comment)).
maflcko
force-pushed
the
Mf1804-ProcessOrphans
branch
3 times, most recently
from
4dee1ea to
246b0d7
Compare
maflcko
force-pushed
the
Mf1804-ProcessOrphans
branch
from
246b0d7 to
0c2bc58
Compare
5 participants
Moving the orphan processing into a new function makes it possible to call it from other places. E.g.
sendrawtransactionor when a transaction is received via aNetMsgTypedifferent fromTX.Also, add test coverage for the function in our
unitfunctional test framework.