Add builder transaction status API #236

dailinsubjam · 2024-11-06T17:47:16Z

Closes #175

This PR:

Add API to claim transaction status.
Set transaction status under different cases.
Add test in legacy builder to check we have correct status recorded for each transaction.
Add pruning by setting target number of transactions we want to save with status.

This PR does not:

Implement or add test for marketplace builder yet.
Add fine-grained pruning yet, like minimum retention or target retention, which will be done next.
Add tests to check transaction submitted through public mempool event stream, it only checks txn submitted from hotshot for now.
Add tests to check other error types besides "TransactionTooBig".
Mark sequenced and record leaf height offset when transaction is sequenced, as it's not a high priority.

Key places to review:

coveralls · 2024-11-06T17:58:48Z

Pull Request Test Coverage Report for Build 12005235276

Details

274 of 318 (86.16%) changed or added relevant lines in 5 files are covered.
3 unchanged lines in 1 file lost coverage.
Overall coverage decreased (-0.07%) to 89.629%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
crates/marketplace/src/service.rs	0	5	0.0%
crates/legacy/src/service.rs	267	306	87.25%

Files with Coverage Reduction	New Missed Lines	%
crates/legacy/src/service.rs	3	92.82%

Totals
Change from base Build 11854071961:	-0.07%
Covered Lines:	6888
Relevant Lines:	7685

💛 - Coveralls

crates/legacy/src/builder_state.rs

crates/legacy/src/service.rs

crates/marketplace/src/service.rs

crates/legacy/src/service.rs

crates/marketplace/src/service.rs

crates/legacy/src/service.rs

QuentinI · 2024-11-21T21:28:34Z

crates/legacy/src/service.rs

+            let old_status = write_guard.get(&txn_hash);
+            match old_status {
+                Some(TransactionStatus::Rejected { reason }) => {
+                    tracing::error!("Changing the status of a rejected transaction to status {:?} is not allowed! The reason it is rejected is {:?}", txn_status, reason);


We will run into most of this cases quite frequently during normal operation, I'd downgrade this to debug.

Why is that? Is this due to a rejected txn could be submitted twice?

Yeah, I'd expect cases where we get duplicates either directly or through hotshot gossiping not to be very rare.
It doesn't indicate that something's wrong with the builder, so I wouldn't categorize this as an error.

I see, then I'd better also not return BuildError in such cases.

Updated in 5b3f1b7.

QuentinI · 2024-11-21T21:32:47Z

crates/legacy/src/service.rs

@@ -180,6 +183,9 @@ pub struct GlobalState<Types: NodeType> {

    pub block_size_limits: BlockSizeLimits,

+    // A mapping from transaction hash to its status
+    pub tx_status: Arc<RwLock<lru::LruCache<Commitment<Types::Transaction>, TransactionStatus>>>,


Is there a reason we want to wrap this in an Arc?

Is this for multi-thread updates? I'll let Sishan confirm/explain though. 😃

Right, I wrapped it in an Arc just in case we need to use spawn with a clone of GlobalState in the future, even though we don’t have such a case yet.

We're wrapping the whole GlobalState in an Arc already in ProxyGlobalState, so that seems superfluous

Updated in ea9e606.

shenkeyao

Mostly lock-related comments.

shenkeyao · 2024-11-22T06:48:58Z

crates/legacy/src/service.rs

@@ -180,6 +183,9 @@ pub struct GlobalState<Types: NodeType> {

    pub block_size_limits: BlockSizeLimits,

+    // A mapping from transaction hash to its status
+    pub tx_status: Arc<RwLock<lru::LruCache<Commitment<Types::Transaction>, TransactionStatus>>>,


Is this for multi-thread updates? I'll let Sishan confirm/explain though. 😃

crates/legacy/src/service.rs

shenkeyao · 2024-11-22T07:01:32Z

crates/legacy/src/service.rs

Can we replace unwrap() calls in this file with better handling unless they are in tests?

Changed to .await? so that any errors will be propagated up the call stack. 490d452.

Commented at the place still with an unwrap.

crates/legacy/src/service.rs

QuentinI · 2024-11-22T18:37:10Z

crates/legacy/src/service.rs

@@ -180,6 +183,9 @@ pub struct GlobalState<Types: NodeType> {

    pub block_size_limits: BlockSizeLimits,

+    // A mapping from transaction hash to its status
+    pub tx_status: Arc<RwLock<lru::LruCache<Commitment<Types::Transaction>, TransactionStatus>>>,


We're wrapping the whole GlobalState in an Arc already in ProxyGlobalState, so that seems superfluous

QuentinI · 2024-11-22T18:40:43Z

crates/marketplace/src/service.rs

+        &self,
+        _txn_hash: Commitment<<Types as NodeType>::Transaction>,
+    ) -> Result<TransactionStatus, BuildError> {
+        todo!("Implement this function");


Can we return an Error here instead of panicking? I know we don't have this deployed yet, but I'd rather not panic in non-testing code anyways

QuentinI · 2024-11-22T18:47:47Z

crates/legacy/src/service.rs

+            let old_status = write_guard.get(&txn_hash);
+            match old_status {
+                Some(TransactionStatus::Rejected { reason }) => {
+                    tracing::error!("Changing the status of a rejected transaction to status {:?} is not allowed! The reason it is rejected is {:?}", txn_status, reason);


Yeah, I'd expect cases where we get duplicates either directly or through hotshot gossiping not to be very rare.
It doesn't indicate that something's wrong with the builder, so I wouldn't categorize this as an error.

crates/legacy/src/service.rs

shenkeyao · 2024-11-23T07:23:16Z

crates/legacy/src/service.rs

Commented at the place still with an unwrap.

dailinsubjam added 2 commits November 6, 2024 00:13

add the mapping of tx to status

b2e3de5

private submit endpoint tx status

de56495

dailinsubjam marked this pull request as draft November 6, 2024 17:47

dailinsubjam added 2 commits November 6, 2024 09:54

fix

d33d118

upd hotshot

711339a

dailinsubjam added 16 commits November 6, 2024 11:59

add status sequenced

0d93478

fmt

a86fd31

add tx_status for marketplace builder

2cf155b

add status pending test

c8f6e65

added tests for txn status rejected

f27062a

test of sequenced tx on test_get_txn_status() added

f731230

merge main

642dc2d

fmt

a4afcb4

valid v

35d1dff

clippy

b34d5e4

wait more time

e610916

fmt

814951f

fmt

185d83e

clean up

35d1aed

clean up

2b9a67a

fmt

05e277c

dailinsubjam marked this pull request as ready for review November 8, 2024 06:23

dailinsubjam requested review from QuentinI, shenkeyao and Ayiga November 8, 2024 06:23

QuentinI suggested changes Nov 8, 2024

View reviewed changes

dailinsubjam added 3 commits November 18, 2024 23:35

merge conflict with build error

7667420

solve merge conflict, build successful

1c193d7

clippy

84d7bc7

dailinsubjam added 5 commits November 21, 2024 15:06

lint

eb5c1e6

fix recursive call

1b5ce7f

not clone the whole tx but only tx commit

c462dfa

do not allow txn status change from sequenced/rejected to other status

b3ee4d5

remove sequenced status

057697b

dailinsubjam requested a review from QuentinI November 21, 2024 14:24

dailinsubjam added 2 commits November 21, 2024 22:44

add test for txn marked status twice

446e571

more tests to cover txn marked twice

458213b

QuentinI reviewed Nov 21, 2024

View reviewed changes

shenkeyao suggested changes Nov 22, 2024

View reviewed changes

dailinsubjam added 5 commits November 22, 2024 20:25

change pending status setting place

5a083a6

remove impl in marketplace

1694935

better dealing with unwrap()

490d452

lock refactor

d94a136

refactor all lock relevant to set_txn_status

87fc9e9

QuentinI reviewed Nov 22, 2024

View reviewed changes

shenkeyao reviewed Nov 23, 2024

View reviewed changes

dailinsubjam added 7 commits November 25, 2024 12:15

remove Arc<> wrap on tx_status

ea9e606

downgrade not_implemented to error

fffda60

allow a rejected tx be changed status

5b3f1b7

update test

7a12d4b

update test

7e3c2cb

remove unwrap()

0a7799f

update unwrap()

b1518b8

dailinsubjam requested review from shenkeyao and QuentinI November 25, 2024 07:32

QuentinI approved these changes Nov 25, 2024

View reviewed changes

shenkeyao approved these changes Nov 26, 2024

View reviewed changes

dailinsubjam merged commit 62b5c43 into main Nov 26, 2024
7 of 8 checks passed

dailinsubjam deleted the sishan/tx_status_api branch November 26, 2024 00:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add builder transaction status API #236

Add builder transaction status API #236

dailinsubjam commented Nov 6, 2024 •

edited

Loading

coveralls commented Nov 6, 2024 •

edited

Loading

QuentinI Nov 21, 2024

dailinsubjam Nov 22, 2024

QuentinI Nov 22, 2024

dailinsubjam Nov 22, 2024

dailinsubjam Nov 25, 2024

QuentinI Nov 21, 2024

shenkeyao Nov 22, 2024

dailinsubjam Nov 22, 2024

QuentinI Nov 22, 2024

dailinsubjam Nov 25, 2024

shenkeyao left a comment

shenkeyao Nov 22, 2024

shenkeyao Nov 22, 2024

dailinsubjam Nov 22, 2024

shenkeyao Nov 23, 2024

QuentinI Nov 22, 2024

QuentinI Nov 22, 2024

dailinsubjam Nov 25, 2024

QuentinI Nov 22, 2024

shenkeyao Nov 23, 2024

Add builder transaction status API #236

Add builder transaction status API #236

Conversation

dailinsubjam commented Nov 6, 2024 • edited Loading

This PR:

This PR does not:

Key places to review:

coveralls commented Nov 6, 2024 • edited Loading

Pull Request Test Coverage Report for Build 12005235276

Details

💛 - Coveralls

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shenkeyao left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dailinsubjam commented Nov 6, 2024 •

edited

Loading

coveralls commented Nov 6, 2024 •

edited

Loading