Skip to content

Improve TxStatusManager tests coverage #2911

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

Merged
merged 2 commits into from
Apr 3, 2025

Conversation

AurelienFT
Copy link
Contributor

@AurelienFT AurelienFT commented Mar 31, 2025

Linked Issues/PRs

Closes #2838.

THIS PR IS A REVIVAL OF #2888 FROM RAFAL. that we can't merge anymore because of CLA. Kudos to RAFAL !

Description

This PR adds some more coverage to the TxStatusManager. Details are available in #2838 (comment).

The two outstanding TODOs should be re-evaluated after we are ready with the ultimate integration test for preconfirmations, because it's likely that this test will cover such cases.

Before requesting review

  • I have reviewed the code myself

Side note as explained above: this is a copy-paste + some reviews comments directly addressed from #2888

@AurelienFT AurelienFT added the no changelog Skip the CI check of the changelog modification label Mar 31, 2025
@AurelienFT AurelienFT self-assigned this Mar 31, 2025
@AurelienFT AurelienFT requested a review from a team March 31, 2025 10:17
Copy link
Member

@MitchTurner MitchTurner left a comment

Choose a reason for hiding this comment

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

Looks good other than one question.

@@ -191,6 +191,8 @@ where
for status in result.tx_status.iter() {
let tx_id = status.id;
let status = from_executor_to_status(block, status.result.clone());

// TODO[RC]: Need a test which fails when the line below is commented.
Copy link
Member

Choose a reason for hiding this comment

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

Does this still need to be done?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nop, it's now catch in integration tests now.

@AurelienFT AurelienFT requested a review from MitchTurner April 2, 2025 14:27
@xgreenx xgreenx merged commit 6fcdd94 into master Apr 3, 2025
34 checks passed
@xgreenx xgreenx deleted the improve_tx_status_maanger_coverage branch April 3, 2025 01:39
@xgreenx xgreenx mentioned this pull request Apr 9, 2025
xgreenx pushed a commit that referenced this pull request Apr 9, 2025
## Version 0.43.0

### Breaking
- [2882](#2882): Changed the
type of the `resolved_outputs` for pre-confirmations. Now it also
includes `Utxoid`. `resolved_outputs` field contains only `Change` and
`Variable` outputs, so the `UtxoId` for them could be hard to derive, if
transaction has known inputs. This information should help to create
dependent transactions more easily.
- [2900](#2900): Get rid of
`Deref` impl on `ImportResult` by introducing wrapper type.
- [2909](#2909): Compressed
block headers now include a merkle root of the temporal registry after
compression was performed.
- [2931](#2931): In
`fuel-core-compression`, the `compress` function now takes a reference
to `Config` instead of the value.

### Added
- [2848](#2848): Link all
components of preconfirmations and add E2E tests.
- [2882](#2882): Listen to tx
status update from `TxStatusManager` in `TxPool`. Added logic to clean
up transactions from the pool if received squeezed out pre
confirmations. Added logic to promote transactions on sentry nodes when
receive pre-confirmation.
- [2885](#2885): Notify P2P
from `TxStatusManager` in case of bad preconfirmation message.
- [2901](#2901): New query
`dryRunRecordStorageReads` which works like `dryRun` but also returns
storage reads, allowing use of execution tracer or local debugger
- [2912](#2912): Add the
`allow_partial` parameter to the `coinsToSpend` query. The default value
of this parameters is `false` to preserve the old behavior. If set to
`true`, the query returns available coins instead of failing when the
requested amount is unavailable.
- [2914](#2914): Tests
ensuring the proof generation and validation of tables with sparse and
merklized blueprints work.

### Changed
- [2859](#2859): Swap out
off-chain worker compression for dedicated compression service in
`fuel-core-bin`.
- [2914](#2914): Break out
test logic to trait methods for `root_storage_tests` and
`basic_merkleized_storage_tests` test macros.
- [2925](#2925): Make
preconfirmation optional on API endpoints.

### Fixed
- [2918](#2918): Only cancel
background work if primary RocksDB instance is dropped
- [2935](#2935): The change
rejects transactions immediately, if they use spent coins. `TxPool` has
a SpentInputs LRU cache, storing all spent coins.

### Removed
- [2859](#2859): Removed DA
compression from off-chain worker in favor of dedicated compression
service in `fuel-core-bin`.

## What's Changed
* Rework `TxStatusManager` tests by @rafal-ch in
#2871
* fix(storage): custom impl of EnumCount for MerkleizedColumn by @rymnc
in #2875
* Update network versions on README by @github-actions in
#2889
* fix(version-compatibility): pin async-graphql to 7.0.15 by @rymnc in
#2891
* Fix proptest for tx status manager by @rafal-ch in
#2893
* fault_proving(compression): glue code for integ into fuel-core by
@rymnc in #2859
* Link and activate preconfirmations and add integrations tests by
@AurelienFT in #2848
* refactor: Get rid of `Deref` impl on `ImportResult` by introducing
wrapper type. by @netrome in
#2900
* fix(compression_service): post merge reviews by @rymnc in
#2895
* Listen status updates from in TxStatusManager in TxPool by @AurelienFT
in #2882
* chore(compression): include registry root in compressed block header
by @rymnc in #2909
* Improve TxStatusManager tests coverage by @AurelienFT in
#2911
* ci(benchmarks): run nightly benchmark and create PR by @rymnc in
#2915
* fix: Only cancel background work if primary RocksDB instance is
dropped by @netrome in #2918
* fix(ci): nightly benchmark by @rymnc in
#2922
* fix(ci): explicitly push before creating PR by @rymnc in
#2926
* Add allow_partial parameter to the "coins to spend" query by
@AurelienFT in #2912
* fix(compression): improve robustness on startup and shutdown by @rymnc
in #2923
* chore(compression): metrics for compression service by @rymnc in
#2920
* chore(compression): pass compression config by reference by @rymnc in
#2931
* Dry run execution tracing by @Dentosal in
#2901
* Make preconfirmation optional on API endpoints by @AurelienFT in
#2925
* Notify P2P in case of bad preconfirmation message by @AurelienFT in
#2885
* chore(1.85): 1.85.0 readiness by @rymnc in
#2937
* Reject transactions for already spent coins by @xgreenx in
#2935
* feat: add tests for sparse and merkleized blueprint proof generation
by @netrome in #2914


**Full Changelog**:
v0.42.0...v0.43.0

---------

Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no changelog Skip the CI check of the changelog modification
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Complete TxStatusManager Service tests
3 participants