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

HIPE for plugin helpers #162

Merged
merged 16 commits into from
Mar 10, 2021
Merged

HIPE for plugin helpers #162

merged 16 commits into from
Mar 10, 2021

Conversation

esplinr
Copy link
Contributor

@esplinr esplinr commented Jan 23, 2021

Introduces features previously discussed in the Indy Contributors calls: frozen ledgers and default fee handlers.

Copy link

@askolesov askolesov left a comment

Choose a reason for hiding this comment

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

Looks wonderful. Just a couple of minor suggestions.

text/0162-plugin-removal-helpers/README.md Outdated Show resolved Hide resolved
text/0162-plugin-removal-helpers/README.md Outdated Show resolved Hide resolved
text/0162-plugin-removal-helpers/README.md Outdated Show resolved Hide resolved
text/0162-plugin-removal-helpers/README.md Outdated Show resolved Hide resolved
text/0162-plugin-removal-helpers/README.md Outdated Show resolved Hide resolved
text/0162-plugin-removal-helpers/README.md Outdated Show resolved Hide resolved
text/0162-plugin-removal-helpers/README.md Outdated Show resolved Hide resolved
text/0162-plugin-removal-helpers/README.md Outdated Show resolved Hide resolved
text/0162-plugin-removal-helpers/README.md Outdated Show resolved Hide resolved
text/0162-plugin-removal-helpers/README.md Outdated Show resolved Hide resolved
text/0162-plugin-removal-helpers/README.md Outdated Show resolved Hide resolved
text/0162-plugin-removal-helpers/README.md Outdated Show resolved Hide resolved
Copy link

@askolesov askolesov left a comment

Choose a reason for hiding this comment

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

Cool. I think the document is ready. Just a couple of optional suggestions from my side.

@esplinr
Copy link
Contributor Author

esplinr commented Jan 26, 2021

@askolesov : I'm concerned that I was misunderstanding when I explained this drawback:

If auth_rules with fees are defined, but no plugin is installed to process them, the validation will fall back to the default fee handler which will ignore them and consequently grant authorization. This permissive behavior could be dangerous in some circumstances, and network administrators should ensure that auth_rules do not rely on fees when no plugin is installed to handle them. This is consistent with other Indy configuration options which are permissive by default to assist new users, but expected to be locked down for production use.

Since we also say:

The default fee handler will contain a copy of the sovtoken fee handler but with validation disabled, so it will allow catchup but will not allow new transactions with fees.

Doesn't that mean that the behavior is not permissive? Meaning all auth_rules that exclusively rely on fees for authorization will always fail unless their is a token plugin to override the default fee handler?

I think I need to rewrite the paragraph about the drawback.

@askolesov
Copy link

@esplinr It's important to distinguish two cases:

  1. Somone tries to execute a transaction that has a fee defined. There will be no logic to decrease the author's balance without plugin so the information that the transaction has fee will be simply ignored. And this is the permissive behaviour you are talking in the first part.
  2. Someone tries to change fee for a transaction (i.e. sends SET_FEE transaction). This is what your second cite is about. In this case, the transaction will be rejected. So yes, it's not permissive behaviour.

Copy link

@m00sey m00sey left a comment

Choose a reason for hiding this comment

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

Reading this I have an overarching feeling it's two HIPEs

one for ledger freezing, a QoL improvement for ledger plugin/development
and the second for the the introduction of fees at an Indy (vs plugin) level

I appreciate both are needed to resolve the current sovtoken issues, but feel they are worth discussing independently as I think they individually provide value

text/0162-plugin-removal-helpers/README.md Outdated Show resolved Hide resolved
### Tutorial
[frozen-ledgers-tutorial]: #frozen-ledgers-tutorial

If a ledger has been created, but will never be used, it can be frozen. The LEDGERS_FREEZE transaction will record the root hashes of one or more ledgers, and perpetually use those hashes when computing audit ledger validity. This has two important advantages:
Copy link

Choose a reason for hiding this comment

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

I understand the sentiment but I don't think it reflects the true origin of this HIPE.

There was every intention for the token ledger to be used initially. Given the nature of ledger plugins, freezing is a "Quality of life" improvement allowing ledgers to be implemented and replaced over time? In this case the token ledger never reached fruition, but future ledgers may become defunct or replaced by a "better version" and I think the language should reflect that.

"If a ledger has been created, but has become defunct or redundant, it can be frozen"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This touches on two separate properties of our blockchain:

  • The ability to achieve consensus when writing a new transaction to the ledger.
  • The ability to audit the history of the ledger to prove that there was no tampering.

When a ledger is frozen, the root hash of the deprecated ledger is available to be used by the audit ledger when computing consensus. But we do not store the entire history of root hashes, so it does not help if someone tries to recompute a history that relies on the removed ledger. The proposed implementation of frozen ledgers only works for Sovrin because MainNet's token ledger was unused, and we don't guarantee historical reliability on StagingNet and BuilderNet.

I expanded on the explanation in the "Drawbacks" section to hopefully make that more clear.

Copy link

Choose a reason for hiding this comment

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

I'll take a look, thank you.

text/0162-plugin-removal-helpers/README.md Outdated Show resolved Hide resolved
text/0162-plugin-removal-helpers/README.md Outdated Show resolved Hide resolved
### Rationale and alternatives
[frozen-ledgers-rationale-and-alternatives]: #frozen-ledgers-rationale-and-alternatives

We considered implementing the frozen ledger hash in a plugin, but we consider it a generally useful feature and believe it should belong in Indy.
Copy link

Choose a reason for hiding this comment

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

I 100% agree with this to me it's a feature of ledger plugins and belongs in Indy

text/0162-plugin-removal-helpers/README.md Outdated Show resolved Hide resolved
text/0162-plugin-removal-helpers/README.md Outdated Show resolved Hide resolved
Signed-off-by: Richard Esplin <[email protected]>
@esplinr esplinr changed the title WIP: First draft of HIPE for plugin helpers HIPE for plugin helpers Jan 30, 2021
Trying to be clear about why only unused ledgers should be dropped,
based on feedback from Kevin W.

Signed-off-by: Richard Esplin <[email protected]>
@esplinr
Copy link
Contributor Author

esplinr commented Jan 30, 2021

For reviewers: a convenient link to the pull request for the Sovrin SIP that is linked in the HIPE.

@esplinr
Copy link
Contributor Author

esplinr commented Feb 2, 2021

PR's that implement the functionality described here:
hyperledger/indy-plenum#1502
hyperledger/indy-node#1641
hyperledger-archives/indy-sdk#2356

Signed-off-by: Richard Esplin <[email protected]>
Copy link
Contributor

@lynnbendixsen lynnbendixsen left a comment

Choose a reason for hiding this comment

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

This HIPE solves the issue in an acceptable way.


If a ledger is added (so its root hash is expected by the audit ledger), but it was never used (so the root hash never changed), then LEDGERS_FREEZE will preserve the third property even when the ledger is removed.

But the third property will not be preserved if there is a history of transactions on a ledger and then the ledger is removed. This is because the LEDGERS_FREEZE transaction only stores the most recent root hash, and not the entire history of root hashes, so that history can not be recreated after the plugin ledger is removed. Instead, the frozen ledger should be retained in its read-only state.
Copy link
Member

Choose a reason for hiding this comment

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

Using the Sovrin Ledgers as an example, the token ledger has been used on BuilderNet and StagingNet, but not on MainNet. Based on this statement it would be safe to delete the token ledger data from MainNet, but not on BuilderNet, or StagingNet. If the ledger were to be deleted from either BuilderNet, or StagingNet the ability to audit the history of the ledger would fail, but how would that present itself, and how do we protect against deleting a ledger that has been used so we can ensure the historical integrity of the ledgers?

Copy link
Member

Choose a reason for hiding this comment

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

Added an issue against the remove_ledger.py script; hyperledger/indy-node#1659

Copy link
Contributor Author

@esplinr esplinr Feb 25, 2021

Choose a reason for hiding this comment

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

HIPE 0162 addresses this in the Drawbacks section.

  1. The ability to audit the history of the ledger to prove that there was no tampering.
    … the third property will not be preserved if there is a history of transactions on a ledger and then the ledger is removed. This is because the LEDGERS_FREEZE transaction only stores the most recent root hash, and not the entire history of root hashes, so that history can not be recreated after the plugin ledger is removed. Instead, the frozen ledger should be retained in its read-only state.

As the HIPE explains at the beginning,

The data stored on the custom ledger created by the plugin will be deleted.
[This] consequence is intended.

The associated SIP makes clear that this is acceptable for the Sovrin test networks because they are not intended to have a permanent history. See my comment on the related sovrin-sip.

Copy link
Member

Choose a reason for hiding this comment

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

The answers focus more heavily on the current intended use case. I'm thinking more about the impact when something like this is used in general and it's impact to historical integrity.

To summarize my understanding so far:

Once the the ledger has been frozen and deleted, the ability to audit the history is irrevocably lost.

  • How does this end up presenting itself from an operational and network administrative standpoint?
  • What are the implications to the network and other ledgers?
  • What is the impact to the network?

If the ledger is frozen but the data is retained, can the history of the ledger be audited?

  • Can this be accomplished if the plugin that created/managed the ledger is removed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't really understand your questions. Perhaps the confusion is due to different uses of the word "audit".

The audit ledger exists to confirm the relationship between all of the ledgers if someone decides to replay the transaction history. But this isn't something that the ledger does. It is something that a third party could theoretically do if desired.

How does this end up presenting itself from an operational and network administrative standpoint?

As described in the "Drawbacks" section, the network continues to function correctly. New transactions can be ordered. Catch-up continues to function, because the validation logic is not executed.

But attempting to reconstruct the ledger by replaying all events will fail because some of the data is missing. Freeze_Ledgers only stored the root hash of the last entry on the deleted ledgers, so you can't calculate the previous root hashes when checking for consistency in the audit table.

What are the implications to the network and other ledgers?

Things work fine, but third parties who try and recreate the ledger can't have confidence that the history wasn't tampered with (because it was tampered with).

What is the impact to the network?

It works fine. Consensus is preserved, new transactions can be ordered, and queries are processed.

If the ledger is frozen but the data is retained, can the history of the ledger be audited?

Sure. All the data is there.

Can this be accomplished if the plugin that created/managed the ledger is removed?

Yes, but it would take some work to figure out what the removed plugin was doing. This is explained in more detail in the HIPE for default fee handlers: "The default fee handler replays transactions without performing validation. This is fine for catching up validator nodes, but an audit of the ledger history would need to take into account the historical validation expected by any removed plugins."

Exactly how to audit a ledger seems to be beyond the scope of this HIPE.

Copy link
Member

Choose a reason for hiding this comment

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

Would this then be a fair summary?

Once a ledger is frozen and the associated plug-in is removed, regardless of whether or not you keep the data, the history of the associated ledger(s) cannot be audited. In order to audit their history you would have to retain the ledger data (not delete the ledger after freezing it) and restore the plugin (or computational equivalent) first.

Copy link
Member

@WadeBarnes WadeBarnes Mar 5, 2021

Choose a reason for hiding this comment

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

How does a frozen ledger affect new nodes added to the network. I assume, whether or not the frozen ledgers data was retained, only the hash at the point in time the ledger was frozen will be replicated to any new nodes. Is this correct? I think this may already be implied by the fact the frozen ledger will not be included in catch up operations.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Would this then be a fair summary?
Once a ledger is frozen and the associated plug-in is removed, regardless of whether or not you keep the data, the history of the associated ledger(s) cannot be audited. In order to audit their history you would have to retain the ledger data (not delete the ledger after freezing it) and restore the plugin (or computational equivalent) first.

I disagree with the sentiment that the history cannot be audited even if you keep the data. The audit process is the same as before, because audits are done outside the ledger software and independent of what plugins are installed on the ledger. To perform an accurate audit you will need to track the history of the software that was installed so that you can confirm the validation rules that were used for the original ledger writes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How does a frozen ledger affect new nodes added to the network. I assume, whether or not the frozen ledgers data was retained, only the hash at the point in time the ledger was frozen will be replicated to any new nodes. Is this correct? I think this may already be implied by the fact the frozen ledger will not be included in catch up operations.

I believe that the ledger data is propagated during catch up to the rest of the pool, even for frozen ledgers. This is because validation is not done for transactions that already have a BLS signature. Is that correct @Toktar ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see this note from @anton.denishchenko: "Frozen ledgers are not catched up (this is possible because the LEDGERS_FREEZE transaction is part of the config ledger, which is applied before additional ledgers)."
Thank you @WadeBarnes for highlighting this misunderstanding. I'll add a note about this topic to the HIPE.

@WadeBarnes
Copy link
Member

We'll continue the conversation from here (hyperledger/indy-node#1659 (comment)) on this PR.

@esplinr
Copy link
Contributor Author

esplinr commented Feb 25, 2021

I believe I have incorporated all the feedback and answered all of the questions.


If a ledger is added (so its root hash is expected by the audit ledger), but it was never used (so the root hash never changed), then LEDGERS_FREEZE will preserve the third property even when the ledger is removed.

But the third property will not be preserved if there is a history of transactions on a ledger and then the ledger is removed. This is because the LEDGERS_FREEZE transaction only stores the most recent root hash, and not the entire history of root hashes, so that history can not be recreated after the plugin ledger is removed. Instead, the frozen ledger should be retained in its read-only state and new nodes in the pool should manually backup the data from frozen ledgers since they are not included in catchup.
Copy link
Member

Choose a reason for hiding this comment

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

and new nodes in the pool should manually backup the data from frozen ledgers since they are not included in catchup.

This would be additional administrative overhead and could easily be overlooked or done incorrectly. It's definitely something that needs to be addressed at some point before too long. I would think It would be best addressed by the existing catch-up process.

How much effort would be required to support catch-up of frozen ledgers? The need for this should at least be captured in a ticket so it's not forgotten.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's too much work for that feature without someone planning to use it, but here is the issue:
hyperledger/indy-plenum#1524

@WadeBarnes WadeBarnes merged commit ee646e9 into hyperledger:master Mar 10, 2021
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.

7 participants