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!: revalidate all outputs #3471

Merged
merged 2 commits into from
Oct 24, 2021
Merged

feat!: revalidate all outputs #3471

merged 2 commits into from
Oct 24, 2021

Conversation

SWvheerden
Copy link
Collaborator

Description

This PR marks all outputs to be invalidated by the validation tasks by removing the:

mined_height,
mined_in_block
mined_mmr_position

Motivation and Context

This will allow a user to revalidate all utxos in the current database.

How Has This Been Tested?

Manual testing that after this has been triggered that all utxo's are invalidated again.

philipr-za
philipr-za reviewed Oct 19, 2021
View reviewed changes
Copy link
Contributor

@philipr-za philipr-za left a comment

Choose a reason for hiding this comment

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

So you also have to do the same process for transactions. This PR only addresses outputs. This PR also just marks them to validated but doesn't trigger a validation.

base_layer/wallet/src/output_manager_service/handle.rs Outdated Show resolved Hide resolved
base_layer/wallet/src/output_manager_service/handle.rs Outdated
@@ -93,6 +94,7 @@ impl fmt::Display for OutputManagerRequest {
GetInvalidOutputs => write!(f, "GetInvalidOutputs"),
GetSeedWords => write!(f, "GetSeedWords"),
ValidateUtxos => write!(f, "ValidateUtxos"),
ReValidateUtxos => write!(f, "ReValidateUtxos"),
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
ReValidateUtxos => write!(f, "ReValidateUtxos"),
RevalidateTxos => write!(f, "RevalidateTxos"),

base_layer/wallet/src/output_manager_service/service.rs Outdated
@@ -274,6 +274,10 @@ where
OutputManagerRequest::ValidateUtxos => {
self.validate_outputs().map(OutputManagerResponse::TxoValidationStarted)
},
OutputManagerRequest::ReValidateUtxos => self
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
OutputManagerRequest::ReValidateUtxos => self
OutputManagerRequest::RevalidateTxos => self

base_layer/wallet/src/output_manager_service/handle.rs Outdated
@@ -127,6 +129,7 @@ pub enum OutputManagerResponse {
SeedWords(Vec<String>),
BaseNodePublicKeySet,
TxoValidationStarted(u64),
TransactionsMarkedToBeRevalidated,
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
TransactionsMarkedToBeRevalidated,
TxoRevalidationStarted,

Copy link
Contributor

Choose a reason for hiding this comment

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

These are not transactions but outputs being validated

base_layer/wallet/src/output_manager_service/service.rs Outdated
@@ -385,6 +389,11 @@ where
Ok(id)
}

async fn revalidate_outputs(&mut self) -> Result<(), OutputManagerError> {
self.resources.db.set_outputs_to_be_revalidated().await?;
Ok(())
Copy link
Contributor

Choose a reason for hiding this comment

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

Trigger a validation as well.

philipr-za
philipr-za reviewed Oct 20, 2021
View reviewed changes
Copy link
Contributor

@philipr-za philipr-za left a comment

Choose a reason for hiding this comment

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

This looks good. I would just like a rust test for both these validations please. The tooling should make it fairly easy to put together.

philipr-za
philipr-za previously approved these changes Oct 21, 2021
View reviewed changes
Copy link
Contributor

@philipr-za philipr-za left a comment

Choose a reason for hiding this comment

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

LGTM

base_layer/wallet/tests/output_manager_service/service.rs

rpc_service_state.set_utxo_query_response(utxo_query_responses.clone());

// This response sets output1 as spent
Copy link
Contributor

Choose a reason for hiding this comment

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

This comment seems wrong? Below it look like both outputs are unspent?

@philipr-za
Copy link
Contributor

Clippy says no

@SWvheerden SWvheerden changed the title feat: revalidate all outputs feat!: revalidate all outputs Oct 21, 2021
philipr-za
philipr-za previously approved these changes Oct 22, 2021
View reviewed changes
Copy link
Contributor

@philipr-za philipr-za left a comment

Choose a reason for hiding this comment

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

LGTM

@aviator-app aviator-app bot removed the mq-failed label Oct 24, 2021
@aviator-app aviator-app bot merged commit 9bd4760 into tari-project:development Oct 24, 2021
sdbondi added a commit to sdbondi/tari that referenced this pull request Oct 25, 2021
@sdbondi
Merge branch 'development' into validator-node-merge-development
9d80b31 
* development: (31 commits)
  feat!: revalidate all outputs (tari-project#3471)
  fix: check SAF message inflight and check stored_at is in past (tari-project#3444)
  feat!: apps should not depend on other app configs (tari-project#3469)
  fix: fix recovery test reporting message (tari-project#3479)
  chore: improve cucumber tests to wait for broadcast (tari-project#3461)
  test: use TCP node for daily sync test (tari-project#3464)
  fix: remove unbounded vec allocations from base node grpc/p2p messaging (tari-project#3467)
  fix: upgrade rustyline dependencies (tari-project#3476)
  fix(dht): discard encrypted message with no destination (tari-project#3472)
  fix: remove consensus breaking change in transaction input (tari-project#3474)
  feat: tx weight takes tariscript and output features into account [igor] (tari-project#3411)
  fix: validate dht header before dedup cache (tari-project#3468)
  fix: sha256sum isn't available on all *nix platforms (tari-project#3466)
  fix: typo in console wallet (tari-project#3465)
  fix: ensure that accumulated orphan chain data is committed before header validation (tari-project#3462)
  fix: remove is_synced check for transaction validation (tari-project#3459)
  feat: improve logging for tari_mining_node (tari-project#3449)
  fix: remove unnecessary wallet dependency (tari-project#3438)
  test: simplify cucumber tests (tari-project#3457)
  ci: create script to update DNS records from hashes.txt (tari-project#3458)
  ...
@SWvheerden SWvheerden deleted the sw_revalidate branch October 27, 2021 08:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stringhandler stringhandler stringhandler approved these changes

@philipr-za philipr-za philipr-za left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@SWvheerden @philipr-za @stringhandler