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

Payloads: add NotValidBefore transaction attribute #2812

Merged
merged 3 commits into from
Mar 11, 2023

Conversation

roman-khimov
Copy link
Contributor

Fixes #1992. Based on #2661 (but fixes a bug already!), completely generic functionality that makes us one little step closer to implementing the notary subsystem.

public override bool Verify(DataCache snapshot, Transaction tx)
{
var block_height = NativeContract.Ledger.CurrentIndex(snapshot);
return block_height >= Height;
Copy link
Member

Choose a reason for hiding this comment

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

It wont wait in the mempool until it's valid

Copy link
Member

Choose a reason for hiding this comment

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

That's the desired behaviour, the transaction is not supposed to wait. It's the sender's responsibility to send it at the right time.

Copy link
Member

Choose a reason for hiding this comment

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

Then... why we need it? the sender can sign it after the desired Height.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

#1992:

It can be used to create some transactions in advance with a guarantee that they won't be accepted until specified block, that property will be used for signature collection service (#1573).

In #1573 context this allows to have some "hostage" transaction that can be used if the main one is not completed in time (which works especially nice with #1991). But in general, any scenario where you can/need to create a transaction now, but you don't want it to be used until some block. Some offline signing scenarios might benefit from it as well.

Copy link
Member

Choose a reason for hiding this comment

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

If the transaction is not valid, it will be included in the know hashes list and it won't be valid until an unknown time

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 not supposed to ever appear on the P2P network (unless it's wrapped into P2PNotaryPayload) until it becomes valid. It can't really appear there before NVB height, any node will reject it right away, so I doubt anyone will even try sending it.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe it's a different problem, but what will happend if it's in known hashes and now it's valid?

How we will know that it was rejected because previously was rejected?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  1. Ordinary transactions are not that much different wrt this problem, if node A sends to node B a tx T that for example is lacking some signatures, then it's gonna be the same after signatures are added to T.
  2. This actually looks like a C# implementation problem as I can leverage it to prevent transaction dissemination through the network, a malicious node can specifically strip witnesses from a valid tx and send it to all peers. The result will depend on the sequence of events, if there are enough bad nodes and if they're quick enough they can effectively block the tx.

@erikzhang
Copy link
Member

I find this attribute useful. But I just think that the same function can be achieved with Signer + Witness.

@roman-khimov
Copy link
Contributor Author

Of course it can be done in the verification script, but that'd mean that you have some different account and it's very inconvenient in many cases (for every NVB height you have to have a different one and you need to push/pull assets to/from it). Also, you have no generic way to get this height (you can only run the script and say valid/invalid), but in fact it's important for #1573.

@roman-khimov
Copy link
Contributor Author

I see it as a natural complement to ValidUntilBlock, btw. Like X.509 "Not valid before"/"Not valid after" pair.

@vncoelho
Copy link
Member

I also find it useful, as @erikzhang highlighted.
However, as @shargon emphasized, in the way it is designed I see many confusions in the sense of waiting NVB height before relaying.

I would prefer if we wait an improvement of this tool until we are able to accept and filter it in the mempool selection.

@superboyiii
Copy link
Member

superboyiii commented Oct 17, 2022

@shargon @erikzhang @roman-khimov Guys @vncoelho, let's move on. I think it's useful, but still need some way to make a final decision since it's in 3.5.0 release list. Maybe we can vote.
❤️ means agree to merge
😕 means disagree

Copy link
Member

@vncoelho vncoelho left a comment

Choose a reason for hiding this comment

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

I see that you are requesting this features in other PRs, right?

As it is right now it is optional, right?
Thus, if we do not add this attribute the TX will not have NVB property and will be kept in the mempool, correct?

@roman-khimov
Copy link
Contributor Author

Sure, it's an attribute, if it's not used it doesn't affect transactions in any way.

@shargon shargon merged commit ec7ac2b into neo-project:master Mar 11, 2023
@roman-khimov roman-khimov deleted the not-valid-before branch March 12, 2023 09:35
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.

"NotValidBefore" transaction attribute type
6 participants