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

feat(feature-activation): add transactions RFC #63

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

glevco
Copy link
Contributor

@glevco glevco commented Aug 9, 2023

Rendered

Acceptance Criteria

  • Add new Feature Activation for Transactions RFC (linked above)
  • Add previous incomplete RFC iterations

ATTENTION: @msbrogli @jansegre it's not necessary to read all 4 files in this PR, only the rendered one linked above. The other two files are incomplete/wrong ideas that are kept just for reference.

@glevco glevco self-assigned this Aug 9, 2023
@glevco glevco force-pushed the feat/feature-activation/transactions branch 4 times, most recently from 4bf4b6a to 488b229 Compare August 12, 2023 16:51
@glevco glevco force-pushed the feat/feature-activation/transactions branch 3 times, most recently from 813a1ed to d50a352 Compare August 21, 2023 16:35
@glevco glevco marked this pull request as ready for review August 21, 2023 16:35

1. A tx is received, and it spends the reward of a block. It is verified for reward lock. Then, there are two possibilities:
1. If the current best block's height IS NOT enough to unlock the reward, the tx is invalid and is rejected.
2. If the current best block's height IS enough to unlock the reward, the tx is valid and remains in the mempool.
Copy link
Member

Choose a reason for hiding this comment

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

I think it's imprecise here. If the next block can confirm the transaction, then the tx is valid and remains in the mempool. Even though the final result is the same, I think that the logic behind this behavior might be relevant for the reader.

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 has been removed from the new iteration.


#### Dealing with new blocks

1. When a block is received, if one of the txs it confirms tries to spend a locked reward, the block is invalid and is rejected.
Copy link
Member

Choose a reason for hiding this comment

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

I think your sentence is confusing. I'd say "should any of the transactions confirmed by the block tries to spend a reward that is still under a lock, then the block is 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.

This has been removed from the new iteration.


1. When a block is received, if one of the txs it confirms tries to spend a locked reward, the block is invalid and is rejected.

This rule is only for guaranteeing no rewards can be spent too early. In practice, it's impossible for such tx to be in the mempool, as it would have been invalidated before by the previous rules. TODO: Is this correct? Why does this rule exist, is it for compatibility with the previous reward lock mechanism?
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 right. It also prevents mining APIs from picking invalid transactions for block templates. Note that any block confirming invalid transactions would be invalid and miners would be wasting hash power.

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 has been removed from the new iteration.

Using the premises above, we must define a function that returns a state, given a transaction (note: we actually return multiple states for multiple features). That function must satisfy the following requirements:

1. All txs that are received after (time-wise) an `ACTIVE` block in the best chain, must also be `ACTIVE`.
2. When all txs are ordered by timestamp, there must not be an `INACTIVE` tx after an `ACTIVE` tx.
Copy link
Member

Choose a reason for hiding this comment

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

Timestamp might be tampered with. You might say that "there must not be an INACTIVE tx after an ACTIVE tx when the transactions are topologically sorted".

Copy link
Contributor Author

@glevco glevco Sep 13, 2023

Choose a reason for hiding this comment

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

This is the only comment that may still be relevant in the new iteration. Tampering of timestamps is accounted for in the RFC, please read the session dedicated to this. This has also been removed.

@glevco glevco force-pushed the feat/feature-activation/transactions branch 3 times, most recently from c31f715 to 2465a8b Compare September 13, 2023 22:43
jansegre
jansegre previously approved these changes Oct 5, 2023
@glevco glevco force-pushed the feat/feature-activation/transactions branch from f41ab7c to ec5eff6 Compare January 23, 2024 20:53
# Summary
[summary]: #summary

This document describes a way to use the Feature Activation process with Transactions, complementing the existing implementation for Blocks. The original [Feature Activation for Blocks RFC](https://github.com/HathorNetwork/rfcs/blob/master/projects/feature-activation/0001-feature-activation-for-blocks.md) can be read for more detailed information on what the Feature Activation process is, why it is necessary, and how it works.
Copy link
Member

Choose a reason for hiding this comment

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

I guess we should add that this design does not address some types of features such as removing OPCODES. The solution for these cases will be addressed in the future.


Then, we arrived at the solution proposed in this document, which has most of the same advantages from the parent block solution, however it does not require any breaking change whatsoever. Here it is: **for a feature, a transaction is considered `ACTIVE` if its closest block, that is, its ancestor block with the greatest height, is `ACTIVE`**. This will be detailed in the Reference-level section.

In other words, transactions today can already point to blocks via their inputs. Only a small portion of transactions spend from blocks, though. The idea is then to propagate this indirect block dependency to all transactions. This can be achieved by simply creating a new metadata that is set when a transaction is received from the network. The calculation is deterministic, equal for all peers, and with constant execution complexity. Also, reorgs are handled automatically by existing code, and no change has to be made in critical components such as the consensus.
Copy link
Member

Choose a reason for hiding this comment

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

I guess this section has some historical background but it does not explain the solution in enough depth. I think we must name the new metadata field and explain what it means giving at least one example. And then explain how this metadata will be used to enable features for transactions.


Repeated here from the Guide-level section:

**For a feature, a transaction is considered `ACTIVE` if its closest block, that is, its ancestor block with the greatest height, is `ACTIVE`.**
Copy link
Member

Choose a reason for hiding this comment

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

I guess we should be careful with the "closest block" expression since it might be misleading. Maybe use the expression closest ancestor block or closest pointed block. I also think that you should expand the explanation to clarify that the transaction itself might be spending a mining reward which will make this block a candidate. I believe that two or three examples should clarify all cases.

Copy link
Member

Choose a reason for hiding this comment

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

I think the proposed mechanism is very good, especially for its simplicity, but I also think "closest block" is not a great expression. I'll think of some alternatives and suggest here.

metadata = self.get_metadata()

if self.is_genesis:
metadata.closest_block = self._settings.GENESIS_BLOCK_HASH
Copy link
Member

Choose a reason for hiding this comment

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

Should we point out this edge case in the description?

this_closest_block = vertex
elif isinstance(vertex, Transaction):
this_closest_block_id = (
self._settings.GENESIS_BLOCK_HASH if vertex.is_genesis else not_none(vertex_meta.closest_block)
Copy link
Member

Choose a reason for hiding this comment

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

I guess you have already handled this edge case above. So here you can safely just do this_closest_block_id = not_none(vertex_meta.closest_block).


A migration will also be necessary to populate this new metadata for existing transactions.

## Example - Releasing new Nano Contracts
Copy link
Member

Choose a reason for hiding this comment

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

Move the example to the Guide-level explanation and make sure the guide-level explanation provide enough details to one can understand the example.

1. Changing behavior in transaction validation. In this case, the usage is straight forward, simply a call to `FeatureService.is_feature_active_for_transaction()` to check whether the new feature is `ACTIVE`.
2. Changing behavior in transaction deserialization. In this case, it may be necessary to create a new transaction validation method that verifies the validity of using a new deserialization feature.

## Mutability of transaction parents
Copy link
Member

Choose a reason for hiding this comment

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

Same here: move to the Guide-level explanation.


Therefore, the mutability of transaction parents is not an issue for the solution proposed in this document.

## Selection of transaction parents
Copy link
Member

Choose a reason for hiding this comment

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

Same here: move to the Guide-level explanation.


This RFC passed through a lot of completely different idea iterations before arriving at this solution. Please see the [iterations directory](./0005-iterations) for more information.

# Prior art
Copy link
Member

Choose a reason for hiding this comment

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

It's missing a Future possibilities where we need to add the ability to enable more advanced features such as removing an OPCODE.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: In Progress (WIP)
Development

Successfully merging this pull request may close these issues.

3 participants