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

fix: tms validation correctly updating #6079

Merged
merged 10 commits into from
Jan 18, 2024
Merged

fix: tms validation correctly updating #6079

merged 10 commits into from
Jan 18, 2024

Conversation

SWvheerden
Copy link
Collaborator

Description

Fixes TMS validation to not keep updating the mined height of coinbase transactions

Motivation and Context

Currently, there is a bug in the coinbase validation where if its not mined, it keeps increasing the mined height.
This changes it to use the OMS to track coinbases as they need to be tracked from the output via the OMS.

@github-actions GitHub Actions
Copy link

github-actions bot commented Jan 16, 2024 •
edited
Loading

Test Results (Integration tests)

29 tests   29 ✅  11m 51s ⏱️
11 suites   0 💤
 2 files     0 ❌

Results for commit 792dbd2.

♻️ This comment has been updated with latest results.

@ghpbot-tari-project ghpbot-tari-project added P-acks_required Process - Requires more ACKs or utACKs P-reviews_required Process - Requires a review from a lead maintainer to be merged labels Jan 16, 2024
@github-actions GitHub Actions
Copy link

github-actions bot commented Jan 16, 2024 •
edited
Loading

Test Results (CI)

1 264 tests   1 264 ✅  11m 18s ⏱️
   39 suites      0 💤
    1 files        0 ❌

Results for commit 89dc816.

♻️ This comment has been updated with latest results.

@SWvheerden SWvheerden changed the title fix: TMS validation correctly updating fix: tms validation correctly updating Jan 16, 2024
hansieodendaal
hansieodendaal reviewed Jan 17, 2024
View reviewed changes
Copy link
Contributor

@hansieodendaal hansieodendaal left a comment

Choose a reason for hiding this comment

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

Looks nice, just some comments.

base_layer/wallet/src/transaction_service/storage/sqlite_db.rs Outdated Show resolved Hide resolved
base_layer/wallet/src/transaction_service/storage/sqlite_db.rs Outdated Show resolved Hide resolved
applications/minotari_app_grpc/proto/wallet.proto Show resolved Hide resolved
base_layer/common_types/src/transaction.rs Show resolved Hide resolved
@hansieodendaal
Copy link
Contributor

It seems this step still needs to be fixed in cucumber for Scenario: Create burn transaction:

All 3 runs failed: Scenario: Create burn transaction: tests/features/WalletTransactions.feature:411:3

   ✘  Then wallet WALLET_A detects all transactions as Mined_or_Faux_Confirmed
      Step failed:
      Defined: tests/features/WalletTransactions.feature:425:5
      Matched: integration_tests/tests/steps/wallet_steps.rs:155:1
      Step panicked. Captured output: Wallet WALLET_A failed to detect tx with tx_id = 3460939629464712155 to be Mined_or_Faux_Confirmed, current status is Broadcast

@SWvheerden
Copy link
Collaborator Author

It seems this step still needs to be fixed in cucumber for Scenario: Create burn transaction:

All 3 runs failed: Scenario: Create burn transaction: tests/features/WalletTransactions.feature:411:3

   ✘  Then wallet WALLET_A detects all transactions as Mined_or_Faux_Confirmed
      Step failed:
      Defined: tests/features/WalletTransactions.feature:425:5
      Matched: integration_tests/tests/steps/wallet_steps.rs:155:1
      Step panicked. Captured output: Wallet WALLET_A failed to detect tx with tx_id = 3460939629464712155 to be Mined_or_Faux_Confirmed, current status is Broadcast

I added a flaky tag, this passes local each time

hansieodendaal
hansieodendaal approved these changes Jan 18, 2024
View reviewed changes
Copy link
Contributor

@hansieodendaal hansieodendaal left a comment

Choose a reason for hiding this comment

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

utACK

@ghpbot-tari-project ghpbot-tari-project removed the P-reviews_required Process - Requires a review from a lead maintainer to be merged label Jan 18, 2024
@SWvheerden SWvheerden merged commit 34222a8 into tari-project:development Jan 18, 2024
10 checks passed
@SWvheerden SWvheerden deleted the sw_tms_fix branch January 18, 2024 12:05
sdbondi added a commit to sdbondi/tari that referenced this pull request Jan 26, 2024
@sdbondi
Merge branch 'development' into feature-dan2-merge
bd818cb 
* development: (84 commits)
  chore: merge development to feature-dan2 (tari-project#6100)
  feat: prevent runtime error with compact error input (tari-project#6096)
  chore(deps): bump h2 from 0.3.21 to 0.3.24 (tari-project#6091)
  chore: update monero to latest release (tari-project#6098)
  chore(deps): bump actions/cache from 3 to 4 (tari-project#6093)
  chore(fix): ci - move s3 uploads to only on release (tari-project#6094)
  feat: update codeowners (tari-project#6088)
  chore: update change log (tari-project#6086)
  feat: add search kernels method to nodejs client (tari-project#6082)
  chore: move domain hash names to separate crate (tari-project#6076)
  chore: new esme release (tari-project#6084)
  fix: tms validation correctly updating (tari-project#6079)
  fix: wallet coinbases not validated correctly (tari-project#6074)
  chore(fix): ci - ffis android - add protobuf for cross-compile (tari-project#6083)
  chore(fix): include protobuf during build (tari-project#6077)
  chore: upgrade tonic and prost (tari-project#6067)
  feat: add tari address as valid string for discovering a peer (tari-project#6075)
  docs: update disclosure policy (tari-project#6072)
  chore(ci): ffi extend to build for none mobile platforms (tari-project#6069)
  refactor(common): allow custom default configs (tari-project#6068)
  ...
sdbondi added a commit to sdbondi/tari that referenced this pull request Jan 26, 2024
@sdbondi
Merge branch 'development' into feature-dan2-merge
cc67bfc 
* development: (85 commits)
  revert: merge development to feature-dan2 (tari-project#6100) (tari-project#6102)
  chore: merge development to feature-dan2 (tari-project#6100)
  feat: prevent runtime error with compact error input (tari-project#6096)
  chore(deps): bump h2 from 0.3.21 to 0.3.24 (tari-project#6091)
  chore: update monero to latest release (tari-project#6098)
  chore(deps): bump actions/cache from 3 to 4 (tari-project#6093)
  chore(fix): ci - move s3 uploads to only on release (tari-project#6094)
  feat: update codeowners (tari-project#6088)
  chore: update change log (tari-project#6086)
  feat: add search kernels method to nodejs client (tari-project#6082)
  chore: move domain hash names to separate crate (tari-project#6076)
  chore: new esme release (tari-project#6084)
  fix: tms validation correctly updating (tari-project#6079)
  fix: wallet coinbases not validated correctly (tari-project#6074)
  chore(fix): ci - ffis android - add protobuf for cross-compile (tari-project#6083)
  chore(fix): include protobuf during build (tari-project#6077)
  chore: upgrade tonic and prost (tari-project#6067)
  feat: add tari address as valid string for discovering a peer (tari-project#6075)
  docs: update disclosure policy (tari-project#6072)
  chore(ci): ffi extend to build for none mobile platforms (tari-project#6069)
  ...
sdbondi added a commit to sdbondi/tari that referenced this pull request Jan 26, 2024
@sdbondi
Merge branch 'development' into feature-dan2-merge
a99eb93 
* development: (85 commits)
  revert: merge development to feature-dan2 (tari-project#6100) (tari-project#6102)
  chore: merge development to feature-dan2 (tari-project#6100)
  feat: prevent runtime error with compact error input (tari-project#6096)
  chore(deps): bump h2 from 0.3.21 to 0.3.24 (tari-project#6091)
  chore: update monero to latest release (tari-project#6098)
  chore(deps): bump actions/cache from 3 to 4 (tari-project#6093)
  chore(fix): ci - move s3 uploads to only on release (tari-project#6094)
  feat: update codeowners (tari-project#6088)
  chore: update change log (tari-project#6086)
  feat: add search kernels method to nodejs client (tari-project#6082)
  chore: move domain hash names to separate crate (tari-project#6076)
  chore: new esme release (tari-project#6084)
  fix: tms validation correctly updating (tari-project#6079)
  fix: wallet coinbases not validated correctly (tari-project#6074)
  chore(fix): ci - ffis android - add protobuf for cross-compile (tari-project#6083)
  chore(fix): include protobuf during build (tari-project#6077)
  chore: upgrade tonic and prost (tari-project#6067)
  feat: add tari address as valid string for discovering a peer (tari-project#6075)
  docs: update disclosure policy (tari-project#6072)
  chore(ci): ffi extend to build for none mobile platforms (tari-project#6069)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hansieodendaal hansieodendaal hansieodendaal approved these changes

Assignees
No one assigned
Labels
P-acks_required Process - Requires more ACKs or utACKs
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@SWvheerden @hansieodendaal @ghpbot-tari-project