feat(feature-activation): add transactions RFC #63
Conversation
glevco
force-pushed
the
feat/feature-activation/transactions
branch
4 times, most recently
from
4bf4b6a to
488b229
Compare
glevco
force-pushed
the
feat/feature-activation/transactions
branch
3 times, most recently
from
813a1ed to
d50a352
Compare
|
|
||
| 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. |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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. |
There was a problem hiding this comment.
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."
There was a problem hiding this comment.
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? |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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. |
There was a problem hiding this comment.
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".
There was a problem hiding this comment.
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
force-pushed
the
feat/feature-activation/transactions
branch
3 times, most recently
from
c31f715 to
2465a8b
Compare
projects/feature-activation/0005-feature-activation-for-transactions.md
Outdated
Show resolved
Hide resolved
projects/feature-activation/0005-feature-activation-for-transactions.md
Outdated
Show resolved
Hide resolved
glevco
force-pushed
the
feat/feature-activation/transactions
branch
from
2465a8b to
5262e37
Compare
glevco
force-pushed
the
feat/feature-activation/transactions
branch
2 times, most recently
from
000f40c to
f41ab7c
Compare
glevco
force-pushed
the
feat/feature-activation/transactions
branch
from
f41ab7c to
ec5eff6
Compare
| # 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. |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
This is mentioned in the Drawbacks section and I added it to Future possibilities, too.
|
|
||
| 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. |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
This is detailed in the Reference-level section and an example is provided in "Example - Releasing new Nano Contracts". I moved the example to the guide level section as requested below.
|
|
||
| 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`.** |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
This was renamed to closest_ancestor_block in the implementation, and I just updated it here.
| metadata = self.get_metadata() | ||
|
|
||
| if self.is_genesis: | ||
| metadata.closest_block = self._settings.GENESIS_BLOCK_HASH |
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
Done, I updated this with the actual implementation which does not include this case.
|
|
||
| A migration will also be necessary to populate this new metadata for existing transactions. | ||
|
|
||
| ## Example - Releasing new Nano Contracts |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
Same here: move to the Guide-level explanation.
There was a problem hiding this comment.
I don't think this should be moved to the guide-level, this is just a consideration on whether the solution has this problem or not.
|
|
||
| Therefore, the mutability of transaction parents is not an issue for the solution proposed in this document. | ||
|
|
||
| ## Selection of transaction parents |
There was a problem hiding this comment.
Same here: move to the Guide-level explanation.
There was a problem hiding this comment.
I also think that this is just an implementation detail that should be in the reference level, not guide level.
|
|
||
| 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 |
There was a problem hiding this comment.
It's missing a Future possibilities where we need to add the ability to enable more advanced features such as removing an OPCODE.
glevco
force-pushed
the
feat/feature-activation/transactions
branch
from
ec5eff6 to
c46bfbd
Compare
Rendered
Acceptance Criteria
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.