Skip to content
This repository was archived by the owner on Feb 17, 2025. It is now read-only.

Fix tx size check#2799

Merged
ToniRamirezM merged 6 commits intorelease/v0.4.1from
fix/txsize
Nov 20, 2023
Merged

Fix tx size check#2799
ToniRamirezM merged 6 commits intorelease/v0.4.1from
fix/txsize

Conversation

@ToniRamirezM
Copy link
Contributor

@ToniRamirezM ToniRamirezM commented Nov 16, 2023

Closes #2798

What does this PR do?

Change the way the tx size is checked

Reviewers

Main reviewers:

@ToniRamirezM ToniRamirezM added this to the v0.4.0 milestone Nov 16, 2023
@ToniRamirezM ToniRamirezM self-assigned this Nov 16, 2023
@cla-bot cla-bot bot added the cla-signed label Nov 16, 2023
@ToniRamirezM ToniRamirezM marked this pull request as draft November 16, 2023 13:10
@ToniRamirezM ToniRamirezM marked this pull request as ready for review November 16, 2023 13:30
@ToniRamirezM ToniRamirezM marked this pull request as draft November 16, 2023 13:47
@ToniRamirezM ToniRamirezM marked this pull request as ready for review November 16, 2023 15:49
if poolTx.Size() > p.cfg.MaxTxBytesSize {
decodedTx, err := state.EncodeTransaction(poolTx.Transaction, 0xFF, p.cfg.ForkID) //nolint: gomnd
if err != nil {
return ErrTxTypeNotSupported
Copy link
Contributor

Choose a reason for hiding this comment

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

isn't it better to return err?

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 error es returned to the user by the RPC, it should be a standard error if possible.

Cost: tx.Cost(),
BatchResources: state.BatchResources{
Bytes: tx.Size(),
Bytes: uint64(len(rawTx)) + state.EfficiencyPercentageByteLength,
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe we need to use state.EncodeTransaction to get the rawTx and pass the forkid to the txTracker, to be coherent

@ToniRamirezM ToniRamirezM modified the milestones: v0.4.0, v0.4.1 Nov 20, 2023
@ToniRamirezM ToniRamirezM changed the base branch from release/v0.4.0 to release/v0.4.1 November 20, 2023 09:34
@ToniRamirezM ToniRamirezM merged commit 0987535 into release/v0.4.1 Nov 20, 2023
@ToniRamirezM ToniRamirezM deleted the fix/txsize branch November 20, 2023 09:52
ToniRamirezM added a commit that referenced this pull request Nov 20, 2023
* fix tx size check

* fix tx size check

* refactor

* fix test

* fix test

* fix
@ToniRamirezM ToniRamirezM added the cherry-picked Content has been cherry-picked into a higher version branch label Nov 20, 2023
ToniRamirezM added a commit that referenced this pull request Nov 20, 2023
* fix tx size check

* fix tx size check

* refactor

* fix test

* fix test

* fix
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

cherry-picked Content has been cherry-picked into a higher version branch cla-signed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants