Plumb events through to TxMeta / LedgerCloseMeta#3516
Plumb events through to TxMeta / LedgerCloseMeta#3516latobarita merged 24 commits intostellar:masterfrom
Conversation
91bcc1a to
b817517
Compare
7dd2847 to
9a65481
Compare
9a65481 to
b731434
Compare
bf1bbc7 to
056673a
Compare
MonsieurNicolas
left a comment
There was a problem hiding this comment.
a few issues but otherwise looks good
MonsieurNicolas
left a comment
There was a problem hiding this comment.
I am pushing a possible fix. We should add a test that does something like closeLedger(*app, {tx1, tx2, tx3}, true); and checks (maybe using the "debug meta" functionality) that we produce the meta that we expect?
|
I pushed a couple commits to that branch:
We should add some tests for this (I suggested in a comment to maybe leverage the "debug stream" in some way: it should be fairly easy to add a few checks against the ledger close meta generated as part of the "meta stream test suite", a ledger containing a fee bump, a regular transaction and maybe a tx that produces events). |
There was a problem hiding this comment.
you forgot to update normalizeMeta in dumpxdr.cpp
There was a problem hiding this comment.
mmm actually for hashes to work properly we have to normalize the meta in protocol (so here)
There was a problem hiding this comment.
(and I'm not sure this can be done more efficiently that what was done in dumpxdr)
|
@MonsieurNicolas I've added a very basic test that does as suggested -- throws some txs at closeLedger and checks the meta is vaguely sensible -- but I'm out of time tonight; will extend it tomorrow, as well as wrap up whatever you mean above in the comment a bout fee bumps, and regenerate the meta test vectors again. |
There was a problem hiding this comment.
we probably don't want to do this if we're not on v=3 (as this has some unknown perf impact)
src/util/MetaUtils.cpp
Outdated
There was a problem hiding this comment.
shouldn't there be a case 3 here?
There was a problem hiding this comment.
Oops! Yes of course, sorry. Also will add a default assert.
There was a problem hiding this comment.
I would add a couple other transactions in there (as the bug we had was that meta was broken with ledgers with more than 1 tx).
For good measure one of these could be a fee bump transaction
There was a problem hiding this comment.
why not just convert to json and do a string comparison of the entire record for "ledger 3"?
That way we'll have in the test the actual expected meta (which will allow us to see when we make any meta changes - as this breaks Horizon).
Also note that vcurrent and vnext have a different format (which we'll see in the test)
There was a problem hiding this comment.
ok, doing so (it adds a lot of test vector files, I eyeballed them a bit but second opinions on whether they're correct welcome!)
74df22d to
1aa67c7
Compare
1aa67c7 to
d0c42b7
Compare
ffb848c to
301bfaa
Compare
|
@MonsieurNicolas at last I believe this is rebased, passing all tests, and (I hope!) addressing all your comments. Should be ready to land. |
|
r+ d4b28ee |
This plumbs ContractEvents through to the preflight endpoint, necessary for #3487
It also plumbs the events through to the LedgerCloseMeta v2 / TransactionMeta v3, as described in CAP-0056.
It also (hopefully correctly) accumulates and hashes the new TransactionResultSetV2 into the ledger header, while still (for now) continuing to publish the old TransactionResultSet values to the database and thus history archives. This will (currently) break offline catchup, which does a download-and-verify job on
resultsfiles in the archives (which, being defined to contain TransactionResultSet, will still load ok but will not verify against the ledger hash), and will also break archivist and likely anything else that tires to check the identity betweenresultsand ledger headers. We should decide how to proceed here before finalizing the design.