-
Notifications
You must be signed in to change notification settings - Fork 969
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
Simulate transaction application #2179
Simulate transaction application #2179
Conversation
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.
only a few minor changes needed
computeContentsHash(Hash const& previousLedgerHash, | ||
std::vector<TransactionEnvelope> transactions) | ||
{ | ||
std::sort( |
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.
it's a bit unfortunate that we're duplicating a lot of code from TxSetFrame
, can we instead just have a TxSetFrame
member that we use to delegate all those things?
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.
The difficulty here is closely related to #2163. I see three options here:
- Leave it as is
- Implement a static template function in
TxSetFrame
that computes the contents hash, with a signature like
template <typename T>
computeContentsHash(
Hash const& previousLedgerHash,
std::vector<T> transactions,
std::function<bool(T const&, T const&)> comp,
std::function<xdr::opaque_vec<>(T const&)> toOpaque);
- Construct an instance of
TxSetFrame
to compute the hash
Given the choices, I prefer 1 or 3. We are already doing something quite like 3 in ApplyTransactionsWork::getNextLedgerFromHistoryArchive
where we construct an instance of TxSetFrame
to get the apply order. My main issue with 3 is that I don't like constructing such a substantial object just to destroy it immediately (I don't like doing it for the apply order either). Let me know what you think?
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.
yeah 3 seems fine for now, the performance impact of it should go away when #2163 is done
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.
Done.
|
||
FileTransferInfo info( | ||
mDownloadDir, HISTORY_FILE_TYPE_LEDGER, | ||
mHistoryManager.nextCheckpointLedger(ledgerSeq + 1) - 1); |
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.
you can use checkpointContainingLedger() method for that
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.
Done.
a1a6574
to
7a988c2
Compare
r+ fc1e49dbe57c8df0dc94a3918e280cac8e9e19c1 |
fc1e49d
to
92b8531
Compare
r+ 92b8531 |
Simulate transaction application Reviewed-by: MonsieurNicolas
This PR adds a
simulate
mode to stellar-core which permits replay of transactions from a history archive at an increased number of operations per ledger. This will be useful for testing the performance characteristics of stellar-core.Checklist
clang-format
v5.0.0 (viamake format
or the Visual Studio extension)