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

ProcessSpecialTxsInBlock should respect fJustCheck #2653

Merged
merged 2 commits into from
Jan 29, 2019

Conversation

UdjinM6
Copy link

@UdjinM6 UdjinM6 commented Jan 27, 2019

We should NOT update internal state in any way (evodb included) when ConnectBlock is called to check block validity only.

Also invoke it after subsidy/payee checks since it could be a heavy one.

@UdjinM6 UdjinM6 added the bug label Jan 27, 2019
@UdjinM6 UdjinM6 added this to the 14.0 milestone Jan 27, 2019
Copy link
Member

@PastaPastaPasta PastaPastaPasta left a comment

Choose a reason for hiding this comment

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

makes sense utACK

@codablock
Copy link

@UdjinM6 I'm wondering why this was needed? The idea with CEvoDb is that it's poor-mans-db-transactions based and in case we only want to check for correctness we simply roll back the db-transaction after we're done, making all changes done to CEvoDb non-persistent.

Look at

auto dbTx = evoDb->BeginTransaction();
for example, it starts the DB transaction but never commits it, which results in rollback when leaving the scope of the method.

Also invoke it after subsidy/payee checks
@UdjinM6
Copy link
Author

UdjinM6 commented Jan 28, 2019

Hmmm..... I would rather not write anything if I know I'm going to drop/rollback it the very next moment but I guess you are right and touching evodb is actually ok-ish (won't it produce some db garbage like storing non-commited txes in some cache btw? 🤔 ).

As for the rest, CDeterministicMNManager::ProcessBlock also triggers NotifyMasternodeListChanged (which deals with governance objects and votes) and CleanupCache and I'm not sure if calling these on block validity test is safe, should probably avoid it.

@codablock
Copy link

@UdjinM6 the DB transactions are in-memory, so there is no load on the disk until commitment. Rolling back simply means discarding the in-memory change-set.

But yeah, triggering NotifyMasternodeListChanged and CleanupCache should probably not happen when fJustCheck==true

Copy link

@codablock codablock left a comment

Choose a reason for hiding this comment

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

utACK

@codablock codablock merged commit 559bdfc into dashpay:develop Jan 29, 2019
@UdjinM6 UdjinM6 deleted the procspectxjustcheck branch November 26, 2020 13:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants