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

Remove redundant BeginBlock #1013

Closed
AdityaSripal opened this issue Feb 25, 2022 · 7 comments
Closed

Remove redundant BeginBlock #1013

AdityaSripal opened this issue Feb 25, 2022 · 7 comments
Labels
nice-to-have testing Testing package and unit/integration tests

Comments

@AdityaSripal
Copy link
Member

Currently, the testing suite will call BeginBlock multiple times in a row. This is not an issue for now, but could become one if the BeginBlock logic ever changes.

The testing suite should be rearchitected to remove redundant BeginBlock calls.

See: https://github.com/cosmos/ibc-go/blob/main/testing/coordinator.go#L191

This will call BeginBlock 3 times in a row

@AdityaSripal AdityaSripal added testing Testing package and unit/integration tests nice-to-have labels Feb 25, 2022
@colin-axner
Copy link
Contributor

I'm running into this call of BeginBlock to be problematic. Might be best to tackle #1014 first so this is trivially removed

@danwt
Copy link

danwt commented May 17, 2022

+1 as finding this problematic

@colin-axner
Copy link
Contributor

short term solution is to avoid that call

I'd like to try to propose a fix in #1265

@danwt
Copy link

danwt commented Jun 6, 2022

In addition to removing the redundant BeginBlock I'm having a related problem:

In many places for a pair of connected chains A, B, it is required to be able to call .GetContext(). But it seems impossible to get the context when the chain only did EndBlock(), and not EndBlock();BeginBlock(). This is unfortunate because you may not wish to BeginBlock.

@AdityaSripal
Copy link
Member Author

I think this is a symptom of how the deliverTxContext is cleared after Endblock on baseapp. Not sure if there's a solution here. Perhaps we could create our own context, but that could allow users to shoot themselves in the foot since it won't mock baseapp's functionality

@danwt
Copy link

danwt commented Jun 7, 2022

how the deliverTxContext is cleared after Endblock

Yeah I think so too

@colin-axner
Copy link
Contributor

This should be closed in the update to ABCI++ ref #3707 as begin, deliver, end block are squashed into a single FinalizeBlock so it wouldn't be possible to redundantly call begin block

Closing preemptively as these changes are inevitable so no need to track this issue

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
nice-to-have testing Testing package and unit/integration tests
Projects
None yet
Development

No branches or pull requests

3 participants