New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Change the way how TxCompletion<TPeer, TAction>
fetch transaction from other peers
#1704
Change the way how TxCompletion<TPeer, TAction>
fetch transaction from other peers
#1704
Conversation
})); | ||
|
||
var validTxs = new List<Transaction<TAction>>(); | ||
IImmutableSet<TxId> stagedTxIds = _blockChain.GetStagedTransactionIds(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BlockChain<T>.StageTransaction()
should do the job of filtering so no additional filtering is necessary at this point. See #1648.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Eh, then again, we can't count validTxs
if we don't do the filtering here. I'll create an issue for this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So you mean L158 to L176 is not required and just
foreach (var tx in txs)
{
try
{
_blockChain.StageTransaction(tx);
}
catch (InvalidTxException ite)
{
const string msg = "Received transaction from {Peer} with id {TxId} " +
"will not be staged since it is invalid.";
_logger.Error(ite, msg, peer, tx.Id);
}
}
will do the work?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll deal with this in #1705.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IStagePolicy
is designed not to throw any Exception
so yes, I think it should be fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But yeah, since broadcasting is tied to validTxs
, I think it should be left as it is for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think suggested logging related changes should suffice for now.
Co-authored-by: Say Cheong <greymistcube@gmail.com>
7a520fd
to
280aae2
Compare
Previously
TxCompletion<TPeer, TAction>.RequestTxsFromPeerAsync()
is maintained maximum 1 task for each peer to fetch transactions from. It caused a bug where fetching does not working even received a newTxId
s from other peer.This PR force to spawn
TxCompletion<TPeer, TAction>.RequestTxsFromPeerAsync()
task every time newTxId
s are received.