-
Notifications
You must be signed in to change notification settings - Fork 19
RLP
redesign
#137
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
base: main
Are you sure you want to change the base?
RLP
redesign
#137
Conversation
Signed-off-by: F Bojarski <[email protected]>
Signed-off-by: F Bojarski <[email protected]>
Signed-off-by: F Bojarski <[email protected]>
Signed-off-by: F Bojarski <[email protected]>
Signed-off-by: F Bojarski <[email protected]>
Signed-off-by: F Bojarski <[email protected]>
Signed-off-by: F Bojarski <[email protected]>
Signed-off-by: F Bojarski <[email protected]>
Signed-off-by: F Bojarski <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
delete all the pattern_v2 folder now I guess ?
rlp_txn/.DS_Store
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
?
\end{enumerate} | ||
The following are \textbf{counter-constant}, \textbf{binary} columns used in particular in the \rlp{}-ization of \textbf{access lists}, | ||
see section~(\ref{rlp txn v2: phase constraints: access list: approach}). | ||
\begin{multicols}{2} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
all this batch of columns should be in compt perspective
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's what I did initially when I was presenting this to you, but I decided against it after consideration: you won't save on columns since the COMPUTATION perspective is already the widest + you get would get extra CONTEXT[i] = 1
pre-conditions all over the place.
$k=1, \dots, \numberOfCounterConstantAuxiliaryDataColumns$: | ||
\ccc; | ||
used to store auxiliary data; | ||
\item |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess this has to disappear now ? seems a duplicate of the previous batch of columns
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a difference: one batch of columns is counter constant, the other isn't. So I split the old TMP_k
column batch in two.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you compile it you'll find that there is a CCC
in the column names. The column description also contain c.c.c.
which stands for "counter constant column".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we have to add the sub phase columns of the accessList ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that's an omission. Fixed it.
|
||
Given the above definitions we require that | ||
\begin{enumerate} | ||
\item \CFI, \typeZeroTx{}, \typeOneTx{}, \typeTwoTx{} be transaction-constant |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
up to type4
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we want replayProtection and yParity to be tx-constant too ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't yet add the extra transaction types, mostly because they weren't included in the initially but also because we have time before including account delegation transactions.
BTW for account delegation transactions we may have to add more binary columns or re-purpose the newly introduced binary columns.
\begin{description} | ||
\item[\underline{\underline{Setting \maxCt{}:}}] | ||
we impose $\maxCt _{i + \relof} = 2$ | ||
\item[\underline{\underline{Setting \rlpTxnSharedColumnIsPrefix{}:}}] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why we want this ?
nBytes = \locRlpPrefixByteSize , | ||
} | ||
\] | ||
\item[\underline{\underline{Imposing nontriviality:}}] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure we need to impse nontrivialoty somewhere ... checking
\begin{description} | ||
\item[\underline{\underline{Setting \maxCt{}:}}] | ||
we impose $\maxCt _{i + \relof} = 2$ | ||
\item[\underline{\underline{Setting \rlpTxnSharedColumnIsPrefix{}:}}] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why do we want this ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The whole purpose of \rlpTxnSharedColumnIsPrefix
has to be re-discussed I believe. In the cases of the INTEGER
, BYTES32
and ADDRESS
we deal with RLP
prefixes simultaneously with the data.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
setting phase_end is missing
\] | ||
We impose the following constraints | ||
\begin{enumerate} | ||
\item we impose |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is true for all the data phase, not only the prefix
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LX and LT are phase constant except for the first phase of a transaction. This was added in a recent commit.
anchorRow = i , | ||
relOffset = 0 , | ||
byteStringLength = \locTransactionPayloadSize , | ||
firstByte = \locFirstByteOrZero , |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we shouldn't constraint to be zero in case of empty.
In case it's empty, the next row will be a transaction row, so exo_data9_(i+1) might me the txn column of a shared register. The easier is to constraint the value only if payload is non empty
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the byteStringLength
is set to 0 what we do with firstByte
will not matter. I'd rather provide it with a value that is indeed a byte value rather than have it undefined, which the above achieves. Am I missing something ?
\[ | ||
\locStartRlpizingNewAddress _{i} \define | ||
\left[ \begin{array}{cl} | ||
\cdot & \rlpTxnSharedColumnIsAccessListAddress _{i} \\ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wouldn't it make more sense to decrement by one when starting the rlp of an access list tuple as we require the address constancy fpor the all tuple ?
\] | ||
The purpose of \locAccessListLengthCountdown{} is to \textbf{initially} hold $\| s( T_\textbf{A} ) \|$ | ||
and then count down to $0$ as the access list gets processed. | ||
\begin{description} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see the initialization ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for the various access list items that make up $T_\textbf{A}$, | ||
and then count down to $0$ as the access list item gets processed. | ||
\begin{description} | ||
\item[\underline{\underline{Updates:}}] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see the initialization ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IIRC there is no explicit initialization: you set the value during the "access list tuple prefix" subphase of IS_ACCESS_LIST
and it decrements throughout the "access list tuple" processing.
We therfore impose that | ||
\begin{enumerate} | ||
\item | ||
$\limbBelongsToBothLtAndLx { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is the case of all the access list, not only this subphase
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I made some changes to the LT
/ LX
constraints so that this setting remains constant throughout every processing phase (except the initial one.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: Macro Argument Labeling Mismatch
The wcpGeneralizedCallToIszero
macro displays incorrect and inconsistent argument labeling. Although the ISZERO
operation takes a single argument, the macro's output labels imply two arguments ("1st argument hi/lo" and "2nd argument hi/lo"). Specifically, the labels "2nd argument hi/lo" are erroneously associated with variables cmdWCP@var@argOneOne
and cmdWCP@var@argOneZero
, which are components of the first argument. All underlying variables (argOneThree
through argOneZero
) consistently indicate a single, multi-limb argument.
pkg/xkeyval_macros/wcp_calls.sty#L146-L148
linea-specification/pkg/xkeyval_macros/wcp_calls.sty
Lines 146 to 148 in b02d5bb
\utt{1st argument lo:} & \cmdWCP@var@argOneTwo \\ | |
\utt{2nd argument hi:} & \cmdWCP@var@argOneOne \\ | |
\utt{2nd argument lo:} & \cmdWCP@var@argOneZero \\ |
pkg/wc3.sty#L142-L149
@@ -18,8 +18,8 @@ | |||
\newcommand{\subPhaseIdDataSize}{\red{83}} | |||
\newcommand{\subPhaseIdTopicDelta}{\red{96}} | |||
|
|||
\newcommand{\subPhaseIdWeightIsPrefix}{\red{6}} | |||
\newcommand{\subPhaseIdWeightisPrefix}{\red{6}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: Macro Naming Inconsistency
The macro \subPhaseIdWeightIsPrefix
was incorrectly renamed to \subPhaseIdWeightisPrefix
in pkg/rlp_log.sty
. This introduces a naming inconsistency, as the lowercase 'i' in isPrefix
deviates from the established IsPrefix
convention used by similar macros (e.g., \subPhaseIdWeightIsOt
, \subPhaseIdWeightIsOd
) and throughout the codebase. This inconsistency is propagated to files like log_info/verticalization.tex
and rlp_txnrcpt/global_constraint.tex
, and appears to be a typo.
No description provided.