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.

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

[occ] Add struct field and helpers for estimate prefills #341

Merged
merged 3 commits into from
Oct 24, 2023

Conversation

udpatil
Copy link
Contributor

@udpatil udpatil commented Oct 24, 2023

Describe your changes and provide context

This adds in the ability to prefill estimates based on metadata passed along with deliverTxBatch

Testing performed to validate your change

Unit Test to verify that multiversion store initialization is now idempotent, and works properly regardless of whether estimate prefill is enabled

@udpatil udpatil changed the base branch from main to occ-main October 24, 2023 13:31
@codecov
Copy link

codecov bot commented Oct 24, 2023

Codecov Report

Merging #341 (d484c72) into occ-main (0b9193c) will decrease coverage by 0.01%.
The diff coverage is 68.00%.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##           occ-main     #341      +/-   ##
============================================
- Coverage     56.09%   56.09%   -0.01%     
============================================
  Files           627      627              
  Lines         52759    52773      +14     
============================================
+ Hits          29597    29604       +7     
- Misses        21060    21065       +5     
- Partials       2102     2104       +2     
Files Coverage Δ
baseapp/abci.go 54.22% <80.00%> (-0.06%) ⬇️
tasks/scheduler.go 78.64% <65.00%> (-1.99%) ⬇️

Copy link
Contributor

@stevenlanders stevenlanders left a comment

Choose a reason for hiding this comment

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

Assuming -1 for incarnation is okay, this looks good to me.


// DeliverTxEntry represents an individual transaction's request within a batch.
// This can be extended to include tx-level tracing or metadata
type DeliverTxEntry struct {
Request abci.RequestDeliverTx
Request abci.RequestDeliverTx
Metadata DeliverTxMetadata
Copy link
Contributor

Choose a reason for hiding this comment

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

2 cents: I'd probably would have just put the Writeset here, because Metadata normally means things like headers/trackers/tracers and not as much real-arguments to the function.

@@ -57,6 +57,7 @@ func (dt *deliverTxTask) Increment() {

// Scheduler processes tasks concurrently
type Scheduler interface {
PrefillEstimates(ctx sdk.Context, metadatas []sdk.DeliverTxMetadata)
Copy link
Contributor

Choose a reason for hiding this comment

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

2 cents: if this always must happen prior to ProcessAll, it may be worth just including the logic in ProcessAll to simplify the interaction.

@udpatil udpatil merged commit 27484e4 into occ-main Oct 24, 2023
15 checks passed
@udpatil udpatil deleted the occ-dep-prefill branch October 24, 2023 15:52
udpatil added a commit that referenced this pull request Jan 2, 2024
## Describe your changes and provide context
This adds in the ability to prefill estimates based on metadata passed
along with deliverTxBatch

## Testing performed to validate your change
Unit Test to verify that multiversion store initialization is now
idempotent, and works properly regardless of whether estimate prefill is
enabled
udpatil added a commit that referenced this pull request Jan 8, 2024
## Describe your changes and provide context
This adds in the ability to prefill estimates based on metadata passed
along with deliverTxBatch

## Testing performed to validate your change
Unit Test to verify that multiversion store initialization is now
idempotent, and works properly regardless of whether estimate prefill is
enabled
udpatil added a commit that referenced this pull request Jan 8, 2024
## Describe your changes and provide context
This adds in the ability to prefill estimates based on metadata passed
along with deliverTxBatch

## Testing performed to validate your change
Unit Test to verify that multiversion store initialization is now
idempotent, and works properly regardless of whether estimate prefill is
enabled
udpatil added a commit that referenced this pull request Jan 18, 2024
## Describe your changes and provide context
This adds in the ability to prefill estimates based on metadata passed
along with deliverTxBatch

## Testing performed to validate your change
Unit Test to verify that multiversion store initialization is now
idempotent, and works properly regardless of whether estimate prefill is
enabled
udpatil added a commit that referenced this pull request Jan 18, 2024
## Describe your changes and provide context
This adds in the ability to prefill estimates based on metadata passed
along with deliverTxBatch

## Testing performed to validate your change
Unit Test to verify that multiversion store initialization is now
idempotent, and works properly regardless of whether estimate prefill is
enabled
udpatil added a commit that referenced this pull request Jan 25, 2024
## Describe your changes and provide context
This adds in the ability to prefill estimates based on metadata passed
along with deliverTxBatch

## Testing performed to validate your change
Unit Test to verify that multiversion store initialization is now
idempotent, and works properly regardless of whether estimate prefill is
enabled
udpatil added a commit that referenced this pull request Jan 31, 2024
## Describe your changes and provide context
This adds in the ability to prefill estimates based on metadata passed
along with deliverTxBatch

## Testing performed to validate your change
Unit Test to verify that multiversion store initialization is now
idempotent, and works properly regardless of whether estimate prefill is
enabled
codchen pushed a commit that referenced this pull request Feb 6, 2024
## Describe your changes and provide context
This adds in the ability to prefill estimates based on metadata passed
along with deliverTxBatch

## Testing performed to validate your change
Unit Test to verify that multiversion store initialization is now
idempotent, and works properly regardless of whether estimate prefill is
enabled
udpatil added a commit that referenced this pull request Feb 27, 2024
## Describe your changes and provide context
This adds in the ability to prefill estimates based on metadata passed
along with deliverTxBatch

## Testing performed to validate your change
Unit Test to verify that multiversion store initialization is now
idempotent, and works properly regardless of whether estimate prefill is
enabled
udpatil added a commit that referenced this pull request Mar 1, 2024
## Describe your changes and provide context
This adds in the ability to prefill estimates based on metadata passed
along with deliverTxBatch

## Testing performed to validate your change
Unit Test to verify that multiversion store initialization is now
idempotent, and works properly regardless of whether estimate prefill is
enabled
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants