Initialize transactions once #2521
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
StephenButtolph
commented
Dec 20, 2023
Comment on lines
-161
to
-175
| func initializeTx(cm codec.Manager, tx *Tx) error { | ||
| signedBytes, err := cm.Marshal(CodecVersion, tx) | ||
| if err != nil { | ||
| return fmt.Errorf("problem creating transaction: %w", err) | ||
| } | ||
|
|
||
| unsignedBytesLen, err := cm.Size(CodecVersion, &tx.Unsigned) | ||
| if err != nil { | ||
| return fmt.Errorf("couldn't calculate UnsignedTx marshal length: %w", err) | ||
| } | ||
|
|
||
| unsignedBytes := signedBytes[:unsignedBytesLen] | ||
| tx.SetBytes(unsignedBytes, signedBytes) | ||
| return nil | ||
| } |
There was a problem hiding this comment.
This is 100% duplicated from tx.Initialize. I have no recollection as to why this was done... but something seems to have been a mistake imo.
Comment on lines
-133
to
-139
| func (p *parser) InitializeTx(tx *Tx) error { | ||
| return initializeTx(p.cm, tx) | ||
| } | ||
|
|
||
| func (p *parser) InitializeGenesisTx(tx *Tx) error { | ||
| return initializeTx(p.gcm, tx) | ||
| } |
There was a problem hiding this comment.
Tx has an Initialize function and we already expose the codec - so these functions aren't needed. Additionally, InitializeTx was only used in tests.
Comment on lines
-92
to
-98
| func (p *parser) InitializeBlock(block Block) error { | ||
| return initialize(block, p.Codec()) | ||
| } | ||
|
|
||
| func (p *parser) InitializeGenesisBlock(block Block) error { | ||
| return initialize(block, p.GenesisCodec()) | ||
| } |
There was a problem hiding this comment.
These functions were never used.
| @@ -88,5 +88,16 @@ func NewStandardBlock( | |||
| Time: uint64(timestamp.Unix()), | |||
| Transactions: txs, | |||
| } | |||
| return blk, initialize(blk, cm) | |||
There was a problem hiding this comment.
Calling initialize here used to call blk.initialize which would re-initialize the transactions. Now we explicitly do not re-initialize the transactions.
dhrubabasu
approved these changes
Dec 20, 2023
danlaine
approved these changes
Dec 20, 2023
abi87
approved these changes
Dec 20, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Why this should be merged
Currently, when creating a block, we re-initialize all transactions being placed into the block. This is inefficient, as the transactions are already initialized. However, this can also introduce racy behavior. In a concurrent setting we generally want to be able to treat transactions as immuable. However calling
.IDon a transaction that could be re-initialized in a different goroutine would be racy.How this works
Ensure that transactions are only initialized once, during creation or parsing.
How this was tested