Skip to content
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

Introduce ledger confirmed_set/any_set classes to separate confirmed-only and confirmed/unconfirmed operations on the ledger #4486

Merged
merged 19 commits into from
Apr 30, 2024

Conversation

clemahieu
Copy link
Contributor

@clemahieu clemahieu commented Mar 12, 2024

The motivation for this PR is in preparation for bounded backlog changes. Currently, both confirmed and unconfirmed transactions are together on disk and the ACID properties of the ledger are encapsulated with a database transaction.

With the bounded backlog, blocks will be stored in memory so there will need to be memory synchronization in addition to database synchronization. Rather than filling the ledger class with unconfirmed/confirmed variants for each operation.

The ledger is divided in to several sets and subsets:
ledger::confirmed - The set of confirmed transactions
ledger::any - The superset of confirmed and unconfirmed transactions

When unconfirmed blocks are held in memory the set ledger::unconfirmed will be added which holds only unconfirmed transactions.

In the future the confirming_set can be moved on to ledger which is the subset of unconfirmed blocks that are queued to be moved in to the confirmed set.

@clemahieu
Copy link
Contributor Author

Changed base file class name to ledger_view_base and renamed the user-facing class to ledger_view.

@clemahieu
Copy link
Contributor Author

Rebased on top of #4484

@clemahieu clemahieu force-pushed the ledger_view branch 10 times, most recently from e8059be to 0655adc Compare March 21, 2024 10:59
@clemahieu clemahieu force-pushed the ledger_view branch 7 times, most recently from 55ac5d9 to 3d681c6 Compare March 27, 2024 12:51
@clemahieu
Copy link
Contributor Author

clemahieu commented Mar 27, 2024

I reworked this from the previous ledger_view design for several reasons:

  • The confirmed/unconfirmed views were almost never interchangeable so using inheritance wasn't really appropriate.
  • Capturing the transaction in the view was never really sufficient so both transaction/view had to be passed around in cases
  • Creating the views littered the code with view variables right next to transaction variables
  • Creating the view had an allocation overhead and many of its uses were short-lived
  • The overwhelming majority of accesses are looking for an unconfirmed view so terseness / syntax sugar is desirable.
  • Fewer operations are needed on the confirmed view than the unconfirmed view which tended to break the inheritence abstraction.

@clemahieu clemahieu marked this pull request as ready for review March 27, 2024 12:59
@clemahieu clemahieu changed the title Introduce ledger_view class separates confirmed/unconfirmed views of the ledger Introduce ledger view classes to separate confirmed/unconfirmed operations on the ledger Mar 27, 2024
@clemahieu clemahieu force-pushed the ledger_view branch 3 times, most recently from 0143f27 to ebe67dd Compare April 4, 2024 14:07
@clemahieu clemahieu force-pushed the ledger_view branch 2 times, most recently from 82d480c to 69c7913 Compare April 16, 2024 04:08
@clemahieu clemahieu changed the title Introduce ledger view classes to separate confirmed/unconfirmed operations on the ledger Introduce ledger confirmed_set/any_set classes to separate confirmed-only and confirmed/unconfirmed operations on the ledger Apr 16, 2024
@clemahieu clemahieu force-pushed the ledger_view branch 4 times, most recently from f7b83ad to c192843 Compare April 22, 2024 08:49
return block_balance > previous_balance.value () ? block_balance.number () - previous_balance.value ().number () : previous_balance.value ().number () - block_balance.number ();
}

// Balance for account containing hash
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment seems wrong, it can be removed since the name of the function is now obvious.


std::optional<nano::amount> nano::ledger_set_any::account_balance (secure::transaction const & transaction, nano::account const & account_a) const
{
auto block = block_get (transaction, ledger.any.account_head (transaction, account_a));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ledger.any seems redundant


public: // Operations on pending entries
std::optional<nano::pending_info> pending_get (secure::transaction const & transaction, nano::pending_key const & key) const;
bool receivable_any (secure::transaction const & transaction, nano::account const & account) const;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be deleted

@clemahieu clemahieu merged commit 5281252 into nanocurrency:develop Apr 30, 2024
25 of 26 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants