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

soroban-rpc: Fix ledgerentry visibility bug #871

Merged
merged 3 commits into from
Aug 21, 2023

Conversation

paulbellamy
Copy link
Contributor

What

Fix a ledger entry visibility bug introduced in #846.

Why

Ledger Entries should be visible when currentLedger == expirationLedgerSeq (according to https://soroban.stellar.org/docs/fundamentals-and-concepts/state-expiration#expiration-ledger). But the code previously made it a bit unclear which ledger was LatestClosed vs Current (which led to the bug).

Known limitations

[TODO or N/A]

@2opremio
Copy link
Contributor

Nice, thanks! Could you also fix the off-by-one printout of TestSimulateTransactionBumpAndRestoreFootprint ?

t.Log("waiting for ledger entry to expire at ledger", entry.MustContractData().ExpirationLedgerSeq+1)

should really be

t.Log("waiting for ledger entry to expire at ledger", entry.MustContractData().ExpirationLedgerSeq)

@paulbellamy paulbellamy enabled auto-merge (squash) August 21, 2023 12:39
@paulbellamy paulbellamy merged commit 06b48ad into main Aug 21, 2023
19 of 21 checks passed
@paulbellamy paulbellamy deleted the fix-ledger-entry-visibility-bug branch August 21, 2023 13:07
@2opremio
Copy link
Contributor

The integration tests failed before merging :S

@2opremio
Copy link
Contributor

I think there is still an off-by-one error, either in the fix or how we use it in compute_restore_footprint_transaction_data_and_min_fee()

@2opremio
Copy link
Contributor

Specifically in:

        if !expirable_entry.has_expired(current_ledger_seq) {
            // noop (the entry hadn't expired)
            continue;
        }

@2opremio
Copy link
Contributor

However, has_expired() seems to be fine;

    fn has_expired(&self, current_ledger_seq: u32) -> bool {
        current_ledger_seq > self.expiration_ledger_seq()
    }

@2opremio
Copy link
Contributor

Can we revert this until we figure it out?

@2opremio
Copy link
Contributor

Specifically it failed at https://github.com/stellar/soroban-tools/actions/runs/5926341294/job/16068360612 with:

2023-08-21T13:15:20.7012344Z     transaction_test.go:251: 
2023-08-21T13:15:20.7013384Z         	Error Trace:	/home/runner/work/soroban-tools/soroban-tools/cmd/soroban-rpc/internal/test/transaction_test.go:251
2023-08-21T13:15:20.7015736Z         	            				/home/runner/work/soroban-tools/soroban-tools/cmd/soroban-rpc/internal/test/simulate_transaction_test.go:785
2023-08-21T13:15:20.7016288Z         	Error:      	Not equal: 
2023-08-21T13:15:20.7016772Z         	            	expected: "SUCCESS"
2023-08-21T13:15:20.7017271Z         	            	actual  : "FAILED"
2023-08-21T13:15:20.7017626Z         	            	
2023-08-21T13:15:20.7017980Z         	            	Diff:
2023-08-21T13:15:20.7018508Z         	            	--- Expected
2023-08-21T13:15:20.7018926Z         	            	+++ Actual
2023-08-21T13:15:20.7019395Z         	            	@@ -1 +1 @@
2023-08-21T13:15:20.7019847Z         	            	-SUCCESS
2023-08-21T13:15:20.7020235Z         	            	+FAILED
2023-08-21T13:15:20.7020897Z         	Test:       	TestSimulateTransactionBumpAndRestoreFootprint

and at simulate_transaction_test.go:785 is where we send the restorefootprint operation

@2opremio 2opremio restored the fix-ledger-entry-visibility-bug branch August 21, 2023 14:17
2opremio added a commit that referenced this pull request Aug 21, 2023
@2opremio 2opremio deleted the fix-ledger-entry-visibility-bug branch August 21, 2023 17:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants