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

New ledger state #1788

Merged
merged 41 commits into from Nov 14, 2018

Conversation

Projects
5 participants
@jonjove
Contributor

jonjove commented Sep 10, 2018

Resolves #1417, replaces #1600

This is a redesigned implementation which resolves many issues discussed in #1600. Some notable design improvements include:

  • Better separation of responsibilities among recursive instances of LedgerState and LedgerStateRoot
  • Better use of RAII to minimize the need of invalidate (now known as deactivate)
  • Removed the notion of forget and replaced it with the notion of a read-only load

More tests are still required and clang-format has not yet been run.

@jonjove jonjove force-pushed the jonjove:new-ledger-state branch from 12b884b to 6f48ab9 Sep 10, 2018

@MonsieurNicolas MonsieurNicolas added this to To Do in v10.1.0 via automation Sep 10, 2018

@MonsieurNicolas

one small step for man...

Show resolved Hide resolved src/invariant/BucketListIsConsistentWithDatabaseTests.cpp
Show resolved Hide resolved src/ledger/LedgerState.cpp
Show resolved Hide resolved src/ledger/LedgerState.h Outdated
Show resolved Hide resolved src/ledger/LedgerState.h
Show resolved Hide resolved src/ledger/LedgerState.h Outdated
Show resolved Hide resolved src/ledger/LedgerState.cpp Outdated
Show resolved Hide resolved src/ledger/LedgerState.cpp Outdated
Show resolved Hide resolved src/ledger/LedgerState.cpp Outdated
Show resolved Hide resolved src/ledger/LedgerState.cpp Outdated
Show resolved Hide resolved src/ledger/LedgerState.cpp Outdated
Show resolved Hide resolved src/ledger/LedgerStateEntry.cpp Outdated
Show resolved Hide resolved src/ledger/LedgerStateEntry.cpp Outdated
Show resolved Hide resolved src/ledger/LedgerStateEntry.h Outdated
Show resolved Hide resolved src/ledger/LedgerStateHeader.cpp Outdated
Show resolved Hide resolved src/ledger/LedgerStateHeader.cpp Outdated
Show resolved Hide resolved src/transactions/TransactionUtils.cpp
Show resolved Hide resolved src/transactions/AllowTrustOpFrame.cpp
Show resolved Hide resolved src/transactions/OfferExchange.cpp
Show resolved Hide resolved src/transactions/OperationFrame.cpp
Show resolved Hide resolved src/transactions/TransactionFrame.cpp Outdated
Show resolved Hide resolved src/main/ApplicationImpl.cpp
Show resolved Hide resolved src/herder/Upgrades.cpp Outdated
Show resolved Hide resolved src/ledger/LedgerManagerImpl.cpp Outdated
Show resolved Hide resolved src/herder/TxSetFrame.h
Show resolved Hide resolved src/test/TxTests.cpp Outdated
Show resolved Hide resolved src/test/TxTests.cpp
Show resolved Hide resolved src/bucket/BucketApplicator.cpp Outdated
LedgerState ls(app->getLedgerStateRoot());

This comment has been minimized.

@MonsieurNicolas

MonsieurNicolas Sep 19, 2018

Contributor

I don't think this test makes sense anymore:

  • it was there to test "round trips" to the database
  • it doesn't do that anymore, so it should be changed to commit for each change - but -
  • tests in LedgerStateTests look very similar to this, and instead we should change them to cover the missing bits:
    • generate more than one object
    • make sure that create + load + delete covers the following
      • delete in a child ls (done already)
      • delete in a sibling (ie both get committed to the database)
      • with and without cache (ie clear cache) to make sure that the database calls work as expected (otherwise we're just testing caches)

This comment has been minimized.

@jonjove

jonjove Nov 7, 2018

Contributor

Removed this test. Added "LedgerState round trip" test that commits several sequential batches of create, modify, and erase.

Show resolved Hide resolved src/main/ApplicationImpl.cpp Outdated
Show resolved Hide resolved src/catchup/VerifyLedgerChainWork.cpp Outdated
Show resolved Hide resolved src/bucket/BucketTests.cpp Outdated
Show resolved Hide resolved src/herder/HerderTests.cpp Outdated
Show resolved Hide resolved src/invariant/LiabilitiesMatchOffersTests.cpp
Show resolved Hide resolved src/ledger/LedgerManagerImpl.cpp
Show resolved Hide resolved src/transactions/ChangeTrustTests.cpp
Show resolved Hide resolved src/transactions/ManageOfferOpFrame.cpp Outdated
Show resolved Hide resolved src/transactions/TransactionFrame.cpp Outdated
Show resolved Hide resolved src/herder/Upgrades.cpp Outdated
Show resolved Hide resolved src/ledger/LedgerManagerImpl.cpp Outdated
@@ -99,7 +99,7 @@ TxSetFramePtr
ApplyLedgerChainWork::getCurrentTxSet()
{
auto& lm = mApp.getLedgerManager();
auto seq = lm.getCurrentLedgerHeader().ledgerSeq;
auto seq = lm.getLastClosedLedgerNum() + 1;

This comment has been minimized.

@vogel

vogel Sep 24, 2018

Contributor

I think it can be good moment to rename getLastClosedLedgerNum to getLastClosedLedgerSeq

This comment has been minimized.

@jonjove

jonjove Nov 7, 2018

Contributor

There are many uses of getLastClosedLedgerNum (I'm seeing 67 on my local branch) and I think changing this would just make this PR even messier. I support the change but let's do it in a separate clean-up PR.

bool hasC =
app->getLedgerManager().getCurrentLedgerVersion() >= 10;
bool hasC = false;
{

This comment has been minimized.

@vogel

vogel Sep 24, 2018

Contributor

I see there are some other uses of this pattern:

LedgerState ls(app->getLedgerStateRoot());
read something from ls.loadHeader().current()

also some of the form:

LedgerState ls(app->getLedgerStateRoot());
write something to ls.loadHeader().current()
ls.commit();

and multiple usages that do not seem to follow any of those patterns.

I'm wondering if you have considered extracting two first into some functions, as introducing new scope just for that seems cumbersome, for example:

auto hasC = readHeader(app).ledgerVersion;
writeHeader(app, [](Heder &h){ h.ledgerVersion = 11; });

I think first one could be useful, not sure if second one is worth it.

This comment has been minimized.

@jonjove

jonjove Nov 7, 2018

Contributor

I think I'd rather leave it as is for now since it makes it very explicit that a LedgerState is being used (this is important for reasoning about deactivation and nesting of ledger states).

Show resolved Hide resolved src/ledger/LedgerState.cpp Outdated
Show resolved Hide resolved src/ledger/LedgerState.cpp Outdated
{
prepareLiabilities(ledgerManager, ld);
header.deactivate();

This comment has been minimized.

@vogel

vogel Sep 25, 2018

Contributor

If I understand it correctly, header needs to be deactivated here only because it does not go out of scope here? If so, could we extract decision if prepareLiabilities should be called to a lambda, avoiding manual deactivation of header?

Like

auto shouldPrepare = [&](){
  auto header = ls.loadHeader();
  uint32_t prevVersion = header.current().ledgerVersion;
  header.current().ledgerVersion = newVersion;
  return header.current().ledgerVersion >= 10 && prevVersion < 10;
}();
if (shouldPrepare) { prepareLiabilities(ls); }

Not sure if that is better, but answer to that question will improve my confidence in how things works now.

This comment has been minimized.

@jonjove

jonjove Sep 25, 2018

Contributor

You are partially correct. header needs to be deactivated here because it does not go out of scope before prepareLiabilities and prepareLiabilities invokes loadHeader. If the condition in the if (...) is not satisfied, then header will be deactivated automatically when it goes out of scope. There is no situation in which header is not deactivated when this function terminates, even in the event of an exception. Some alternatives to this implementation include:

  1. Load header in a scope and perform checks in the scope (analogous to your lambda solution)
  2. Pass header to prepareLiabilities so it does not need to be loaded again
  3. Add a nested LedgerState at the beginning of prepareLiabilities

I see no advantage to options 1 or 3. But option 2 would avoid calls to loadHeader in prepareLiabilities, which as currently implemented has been used a number of times. Let me know if you think this would be better.

This comment has been minimized.

@vogel

vogel Sep 26, 2018

Contributor

I think option 2. is OK, especially as prepareLiabilities is only called when we already have header instance.

This comment has been minimized.

@jonjove

jonjove Nov 7, 2018

Contributor

Done.

Show resolved Hide resolved src/ledger/LedgerState.cpp Outdated
Show resolved Hide resolved src/ledger/LedgerState.cpp Outdated
mActive.clear();
mActiveHeader.reset();
mParent.rollbackChild(id);

This comment has been minimized.

@vogel

vogel Sep 25, 2018

Contributor

this looks a bit weird
State in its own rollback initiates parent->rollbackChild, and its name suggest that it will do rollback on this State object, which parent does not do. What about renaming rollbackChild to clearChild or resetChild?

This comment has been minimized.

@jonjove

jonjove Nov 7, 2018

Contributor

I agree the naming is a little strange, but I chose this name since rollbackChild is to rollback as commitChild is to commit. I'm not sure if clearChild or resetChild are much better since they also imply that they are doing something to the child, which they are not.

Show resolved Hide resolved src/ledger/LedgerState.cpp Outdated
auto header = mChild->getHeader();
auto entries = mChild->getEntries();
mBestOffersCache->clear();

This comment has been minimized.

@vogel

vogel Sep 25, 2018

Contributor

should we also clear that on rollbackChild?

This comment has been minimized.

@jonjove

jonjove Nov 7, 2018

Contributor

There is no need to clear mBestOffersCache on rollbackChild. If rollbackChild occurs, it is guaranteed that the database has not been modified since the child was created. Since mBestOffersCache reflects only what was in the database at the time the child was created, it must still be valid after rollbackChild.

Show resolved Hide resolved src/ledger/LedgerManagerImpl.cpp Outdated
Show resolved Hide resolved src/ledger/LedgerManagerImpl.cpp
Show resolved Hide resolved src/ledger/LedgerManagerImpl.cpp
Show resolved Hide resolved src/ledger/LedgerState.cpp Outdated
Show resolved Hide resolved src/ledger/LedgerState.cpp
@MonsieurNicolas

Great progress; I marked as resolved everything I could (@vogel should do the same).

Comments left are the ones that I think are not addressed yet, I think biggest change left are extra tests/deletion of some tests (after exception safety that you're already working on).

Show resolved Hide resolved src/ledger/LedgerStateImpl.h
Show resolved Hide resolved src/ledger/LedgerState.cpp Outdated
Show resolved Hide resolved src/ledger/LedgerState.cpp Outdated
Show resolved Hide resolved src/ledger/LedgerState.cpp
Show resolved Hide resolved src/ledger/LedgerState.cpp
Show resolved Hide resolved src/ledger/LedgerState.cpp Outdated
Show resolved Hide resolved src/ledger/LedgerState.cpp Outdated
Show resolved Hide resolved src/ledger/LedgerState.cpp
Show resolved Hide resolved src/ledger/LedgerState.cpp Outdated
Show resolved Hide resolved src/ledger/LedgerState.cpp
Show resolved Hide resolved src/ledger/LedgerStateImpl.h Outdated
Show resolved Hide resolved src/ledger/LedgerStateOfferSQL.cpp Outdated
Show resolved Hide resolved src/ledger/LedgerManagerImpl.cpp Outdated

@jonjove jonjove force-pushed the jonjove:new-ledger-state branch from a4638ca to 3509cf1 Nov 13, 2018

@MonsieurNicolas

This comment has been minimized.

Contributor

MonsieurNicolas commented Nov 13, 2018

r+ 3509cf1

@latobarita

This comment has been minimized.

Contributor

latobarita commented on 3509cf1 Nov 13, 2018

saw approval from MonsieurNicolas
at jonjove@3509cf1

This comment has been minimized.

Contributor

latobarita replied Nov 13, 2018

merging jonjove/stellar-core/new-ledger-state = 3509cf1 into auto

This comment has been minimized.

Contributor

latobarita replied Nov 13, 2018

jonjove/stellar-core/new-ledger-state = 3509cf1 merged ok, testing candidate = 37d5bde

This comment has been minimized.

This comment has been minimized.

Contributor

latobarita replied Nov 14, 2018

fast-forwarding master to auto = 37d5bde

latobarita added a commit that referenced this pull request Nov 13, 2018

Merge pull request #1788 from jonjove/new-ledger-state
New ledger state

Reviewed-by: MonsieurNicolas

@latobarita latobarita merged commit 3509cf1 into stellar:master Nov 14, 2018

1 check passed

default all tests passed
Details

v10.1.0 automation moved this from In progress to Done Nov 14, 2018

@jonjove jonjove referenced this pull request Nov 14, 2018

Merged

New ledger state fixes #1830

@oryband oryband referenced this pull request Nov 15, 2018

Merged

Anti-SPAM features #3

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment