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.

Sign up for GitHub

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

Jump to bottom

feat: metric for counting rejected transactions #1415

Merged
merged 5 commits into from
Jul 31, 2024
Merged

feat: metric for counting rejected transactions #1415

merged 5 commits into from
Jul 31, 2024

Conversation

Eoous
Copy link
Contributor

@Eoous Eoous commented Jul 14, 2024 •
edited
Loading

Description

Add counter to the block executor metrics :

  • RejectedTransactions

Resolves #1241

Unblocks #1007

PR checklist

  • Tests written/updated
  • Changelog entry added in .changelog (we use
    unclog to manage our changelog)
  • Updated relevant documentation (docs/ or spec/) and code comments

@Eoous Eoous requested a review from a team as a code owner July 14, 2024 22:53
@Eoous Eoous requested review from ramin and rach-id and removed request for a team July 14, 2024 22:53
rach-id
rach-id previously approved these changes Jul 15, 2024
View reviewed changes
Copy link
Member

@rach-id rach-id left a comment

Choose a reason for hiding this comment

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

utAck

@evan-forbes
Copy link
Member

evan-forbes commented Jul 15, 2024 •
edited
Loading

thanks for this @0xEclair ! fwiw, I don't think we can close #1007, as closing that issue requires reaping more txs than what is currently being reaped. This does unblock making a good decision there however as we will get more information on how big of an issue this

just edited to stop us from closing that issue prematurely 🙂

@Eoous
Copy link
Contributor Author

Eoous commented Jul 15, 2024

TYSM. It is indeed so. For closing #1007, we need further discussion and testing

evan-forbes
evan-forbes reviewed Jul 15, 2024
View reviewed changes
state/execution.go Outdated Show resolved Hide resolved
cmwaters
cmwaters reviewed Jul 19, 2024
View reviewed changes
Copy link
Contributor

@cmwaters cmwaters left a comment

Choose a reason for hiding this comment

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

I think if we want to count the amount of rejected transactions, this needs to be done on the celestia-app side within PrepareProposal. I still think this is an interesting metric to have. It could be worth further investigation if there are a large amount of transactions that are included in a block but fail to be executed but this serves an orthogonal purpose

@Eoous Eoous changed the title feat: metric for counting transactions rejected by application feat: metric for counting failed transactions Jul 19, 2024
@Eoous
Copy link
Contributor Author

Eoous commented Jul 19, 2024

Thanks for commenting. I'll try to fix it for closing #1241 from celestia-app side.

@Eoous
feat: metric for counting transactions rejected by application
70c0a48 
Revert "feat: metric for counting transactions rejected by application"

This reverts commit d64ebd7.

feat: counter for rejected transactions
@Eoous
chore: change order
a8fd2f2
@Eoous
Copy link
Contributor Author

Eoous commented Jul 19, 2024

@cmwaters maybe we can fix it within CreateProposalBlock inside celestia-core

@rootulp rootulp removed the request for review from ramin July 19, 2024 15:32
@Eoous Eoous changed the title feat: metric for counting failed transactions feat: metric for counting rejected transactions Jul 19, 2024
@cmwaters
Copy link
Contributor

@cmwaters maybe we can fix it within CreateProposalBlock inside celestia-core

You could do something like count the length of the txs that is given to PrepareProposal and compare that with the length of the transactions returned

@Eoous
Copy link
Contributor Author

Eoous commented Jul 22, 2024

Yeah. Please review new codes

evan-forbes
evan-forbes reviewed Jul 24, 2024
View reviewed changes
Copy link
Member

@evan-forbes evan-forbes left a comment

Choose a reason for hiding this comment

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

nice, we still need to change one or two very minor things for this metric to be accurate, but we're very close! thanks for this @0xEclair

state/execution.go Outdated Show resolved Hide resolved
cmwaters
cmwaters approved these changes Jul 26, 2024
View reviewed changes
Copy link
Contributor

@cmwaters cmwaters left a comment

Choose a reason for hiding this comment

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

👍

@cmwaters
Copy link
Contributor

Going to try port this over to v0.34.x-celestia

@cmwaters cmwaters merged commit 025a8bc into celestiaorg:main Jul 31, 2024
14 of 15 checks passed
@gitpoap-bot gitpoap-bot
Copy link

gitpoap-bot bot commented Jul 31, 2024

Congrats, your important contribution to this open-source project has earned you a GitPOAP!

GitPOAP: 2024 Celestia Contributor:

GitPOAP: 2024 Celestia Contributor GitPOAP Badge

Head to gitpoap.io & connect your GitHub account to mint!

Learn more about GitPOAPs here.

cmwaters added a commit that referenced this pull request Jul 31, 2024
@Eoous @cmwaters
feat: metric for counting rejected transactions (#1415)
4a69a7a 
Add counter to the block executor metrics :
- RejectedTransactions

Resolves #1241

Unblocks #1007

- [ ] Tests written/updated
- [ ] Changelog entry added in `.changelog` (we use
[unclog](https://github.com/informalsystems/unclog) to manage our
changelog)
- [ ] Updated relevant documentation (`docs/` or `spec/`) and code
comments

---------

Co-authored-by: 0xEclair <[email protected]>
Co-authored-by: Callum Waters <[email protected]>
@cmwaters cmwaters mentioned this pull request Jul 31, 2024
Merged
evan-forbes pushed a commit that referenced this pull request Jul 31, 2024
@Eoous @cmwaters @evan-forbes
feat: metric for counting rejected transactions (#1415)
84ea5f3 
Add counter to the block executor metrics :
- RejectedTransactions

Resolves #1241

Unblocks #1007

- [ ] Tests written/updated
- [ ] Changelog entry added in `.changelog` (we use
[unclog](https://github.com/informalsystems/unclog) to manage our
changelog)
- [ ] Updated relevant documentation (`docs/` or `spec/`) and code
comments

---------

Co-authored-by: 0xEclair <[email protected]>
Co-authored-by: Callum Waters <[email protected]>
cmwaters added a commit that referenced this pull request Aug 1, 2024
@cmwaters @Eoous @rootulp
feat: metric for counting rejected transactions (port #1415) (#1445)
6333e56 
Add counter to the block executor metrics :
- RejectedTransactions

Port of #1415

---------

Co-authored-by: Eoous <[email protected]>
Co-authored-by: 0xEclair <[email protected]>
Co-authored-by: Rootul P <[email protected]>
rach-id pushed a commit that referenced this pull request Nov 18, 2024
@cmwaters @Eoous
@rootulp @rach-id
feat: metric for counting rejected transactions (port #1415) (#1445)
2ea098e 
Add counter to the block executor metrics :
- RejectedTransactions

Port of #1415

---------

Co-authored-by: Eoous <[email protected]>
Co-authored-by: 0xEclair <[email protected]>
Co-authored-by: Rootul P <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@evan-forbes evan-forbes evan-forbes approved these changes

@cmwaters cmwaters cmwaters approved these changes

@rach-id rach-id Awaiting requested review from rach-id

Assignees
No one assigned
Labels
S:backport-to-v0.34.x
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Track how many transactions are rejected by the application as a metric
4 participants
@Eoous @evan-forbes @cmwaters @rach-id