Additional rpcs #535

purusang · 2024-12-12T15:54:12Z

Description

These additional rpcs are aimed to be helpful for debugging purpose.

strata_getBlockById
strata_getBlocksAtIdx
strate_getChainstateAtIdx
strata_getClientStateAtIdx
strata_getLastBroadcastEntry
strata_getBroadcastEntryByIdx

Type of Change

Bug fix (non-breaking change which fixes an issue)
New feature/Enhancement (non-breaking change which adds functionality or enhances an existing one)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
Documentation update
Refactor
New or updated tests
Dependency Update

Notes to Reviewers

Looking forward to get suggestions on fields to be exposed from L2Block::body and ChainState.

Checklist

I have performed a self-review of my code.
I have commented my code where necessary.
I have updated the documentation if needed.
My changes do not introduce new warnings.
I have added tests that prove my changes are effective or that my feature works.
New and existing tests pass with my changes.
I have tested them locally.

Related Issues

STR-801

codecov · 2024-12-12T16:12:24Z

Codecov Report

Attention: Patch coverage is 3.47826% with 111 lines in your changes missing coverage. Please review.

Project coverage is 57.13%. Comparing base (506fd5c) to head (5b5d5fe).
Report is 5 commits behind head on main.

Files with missing lines	Patch %	Lines
bin/strata-client/src/rpc_server.rs	0.00%	63 Missing ⚠️
crates/btcio/src/broadcaster/handle.rs	0.00%	6 Missing ⚠️
crates/storage/src/ops/l1tx_broadcast.rs	0.00%	6 Missing ⚠️
crates/rocksdb-store/src/broadcaster/db.rs	0.00%	5 Missing ⚠️
crates/state/src/da_blob.rs	0.00%	5 Missing ⚠️
crates/rpc/types/src/types.rs	0.00%	4 Missing ⚠️
crates/state/src/exec_update.rs	20.00%	4 Missing ⚠️
bin/strata-client/src/main.rs	0.00%	3 Missing ⚠️
crates/state/src/block.rs	25.00%	3 Missing ⚠️
crates/state/src/l1/tx.rs	0.00%	3 Missing ⚠️
... and 7 more

@@            Coverage Diff             @@
##             main     #535      +/-   ##
==========================================
- Coverage   57.33%   57.13%   -0.20%     
==========================================
  Files         309      308       -1     
  Lines       31510    31584      +74     
==========================================
- Hits        18067    18047      -20     
- Misses      13443    13537      +94

Files with missing lines	Coverage Δ
crates/consensus-logic/src/csm/state_tracker.rs	`45.60% <ø> (ø)`
crates/db/src/traits.rs	`0.00% <ø> (ø)`
crates/db/src/types.rs	`54.54% <0.00%> (ø)`
crates/primitives/src/l1.rs	`77.40% <0.00%> (ø)`
crates/rpc/api/src/lib.rs	`0.00% <0.00%> (ø)`
crates/state/src/batch.rs	`35.50% <0.00%> (ø)`
crates/state/src/tx.rs	`80.00% <66.66%> (ø)`
crates/state/src/header.rs	`85.14% <0.00%> (ø)`
crates/state/src/l1/header.rs	`76.31% <0.00%> (ø)`
bin/strata-client/src/main.rs	`0.00% <0.00%> (ø)`
... and 9 more

... and 12 files with indirect coverage changes

crates/rpc/types/src/types.rs

crates/rpc/api/src/lib.rs

delbonis · 2024-12-13T15:46:09Z

Also should take a look at STR-697, as this overlaps.

delbonis

As a rule of thumb, if implementing some feature involves reaching down multiple layers and adding new functionality deep down, then either some more discussion is needed to properly scope the ticket and design the new functionality better or there's a misunderstanding somewhere.

bin/strata-client/src/rpc_server.rs

crates/db/src/traits.rs

crates/db/src/stubs/chain_state.rs

crates/rocksdb-store/src/chain_state/db.rs

crates/db/src/traits.rs

crates/rocksdb-store/src/chain_state/db.rs

bin/strata-client/src/rpc_server.rs

- strata_getBlockById - strata_getBlocksAtIdx - strata_blockNumber - strata_getLastChainstateIdx - strata_getLastClientStateIdx - strata_getClientStateAtIdx TODO: - strata_getLastBroadcastEntry - strata_getBroadcastEntryByIdx - strate_getChainstateAtIdx

* strata_getLastBroadcastEntry * strata_getBroadcastEntryByIdx * strata_getChainstateAtIdx

* removed functionally duplicating rpcs * exposed L2Block through debug namespace

fix: reconstruction of state at rpc_server instead of fetching using db fix: get_broadcast_entry_by_idx, mistakenly txid was used to fetch, now corrected

bewakes

Looks good, minor nits.

bewakes · 2024-12-31T06:33:22Z

crates/rpc/api/src/lib.rs

+    #[method(name = "strata_getLastBroadcastEntry")]
+    async fn get_last_broadcast_entry(&self) -> RpcResult<Option<L1TxEntry>>;
+
+    /// Get the broadcast entry by its id


Suggested change

/// Get the broadcast entry by its id

/// Get the broadcast entry by its idx

bewakes · 2024-12-31T06:35:38Z

crates/rpc/api/src/lib.rs

+    #[method(name = "getChainstateAtIdx")]
+    async fn get_chainstate_at_idx(&self, idx: u64) -> RpcResult<Option<RpcChainState>>;
+
+    /// Get the ClientState at a certain index
+    #[method(name = "getClientStateAtIdx")]
+    async fn get_clientstate_at_idx(&self, idx: u64) -> RpcResult<Option<ClientState>>;


Should these also be a part of debug RPC?

Also, would it make sense to have a new type RpcClientState instead of exposing the ClientState if this is not a part of debug RPC?

I think it does make sense to have them in debug_. The notion behind types Rpc* is due to Serialization trait not being derived for those structs or to have a stripped down version of the struct. If we move these to debug then this should not be an issue.
cc: @delbonis

bewakes · 2024-12-31T06:37:05Z

crates/rocksdb-store/src/broadcaster/db.rs

@@ -82,6 +82,14 @@ impl L1BroadcastDatabase for L1BroadcastDb {
            )))
        }
    }
+
+    fn get_last_broadcast_entry(&self) -> DbResult<Option<L1TxEntry>> {


We should also add a corresponding unit test for this.

also, feels like the name should be get_last_tx_entry ?

bewakes · 2024-12-31T06:40:55Z

bin/strata-client/src/rpc_server.rs

+    }
+
+    async fn get_broadcast_entry_by_idx(&self, idx: u64) -> RpcResult<Option<L1TxEntry>> {
+        let broadcast_handle = self.broadcast_handle.clone();


Probably don't need to clone here and above?

bewakes · 2024-12-31T06:42:25Z

bin/strata-client/src/rpc_server.rs

+            .get_block_async(&block_id)
+            .await
+            .map_err(Error::Db)?
+            .map(|b| b.block().clone());


Not sure but maybe we don't need a clone here?

.block() returns reference so it's needed in this case.

bewakes · 2024-12-31T06:43:07Z

bin/strata-client/src/rpc_server.rs

+#[async_trait]
+impl StrataDebugApiServer for StrataDebugRpcImpl {
+    async fn get_block_by_id(&self, block_id: L2BlockId) -> RpcResult<Option<L2Block>> {
+        let l2_block_manager = self.l2_block_manager.clone();


purusang · 2025-01-01T03:24:55Z

In a discussion with @delbonis, regarding debug apis, he suggested a nice idea of returning native byte representation as hex so that we do not need additional Serialize. What are your thoughts on it @bewakes @sapinb?. I'm positive about it.

delbonis requested changes Dec 12, 2024

View reviewed changes

purusang self-assigned this Dec 13, 2024

purusang force-pushed the STR-714-additional-rpc branch 2 times, most recently from 0885748 to 14a9619 Compare December 18, 2024 13:00

purusang requested review from delbonis and sapinb December 18, 2024 15:32

delbonis requested changes Dec 19, 2024

View reviewed changes

purusang requested review from delbonis and bewakes December 23, 2024 12:05

delbonis requested changes Dec 23, 2024

View reviewed changes

bin/strata-client/src/rpc_server.rs Outdated Show resolved Hide resolved

purusang requested a review from delbonis December 24, 2024 05:46

purusang added 5 commits December 24, 2024 11:37

Added RPC endpoints

c8570b0

- strata_getBlockById - strata_getBlocksAtIdx - strata_blockNumber - strata_getLastChainstateIdx - strata_getLastClientStateIdx - strata_getClientStateAtIdx TODO: - strata_getLastBroadcastEntry - strata_getBroadcastEntryByIdx - strate_getChainstateAtIdx

additional rpcs

0379cf4

* strata_getLastBroadcastEntry * strata_getBroadcastEntryByIdx * strata_getChainstateAtIdx

refactor: additional rpcs

2fc3caa

* removed functionally duplicating rpcs * exposed L2Block through debug namespace

cleanup: removed RpcL2Block

887bf95

cleanup: removal of get_state_at from db

d7fb48d

fix: reconstruction of state at rpc_server instead of fetching using db fix: get_broadcast_entry_by_idx, mistakenly txid was used to fetch, now corrected

purusang force-pushed the STR-714-additional-rpc branch from 90abdc6 to ebabc76 Compare December 24, 2024 05:52

fix: get_clientstate_at_idx made to use wait_blocking

5b5d5fe

purusang force-pushed the STR-714-additional-rpc branch from ebabc76 to 5b5d5fe Compare December 28, 2024 05:10

purusang marked this pull request as ready for review December 28, 2024 05:40

purusang requested review from a team as code owners December 28, 2024 05:40

bewakes reviewed Dec 31, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Additional rpcs #535

Additional rpcs #535

purusang commented Dec 12, 2024 •

edited by john-light

Loading

codecov bot commented Dec 12, 2024 •

edited

Loading

delbonis commented Dec 13, 2024

delbonis left a comment

bewakes left a comment

bewakes Dec 31, 2024

bewakes Dec 31, 2024

purusang Dec 31, 2024

bewakes Dec 31, 2024

purusang Jan 1, 2025

bewakes Dec 31, 2024

bewakes Dec 31, 2024

purusang Dec 31, 2024

bewakes Dec 31, 2024

purusang commented Jan 1, 2025 •

edited

Loading

	/// Get the broadcast entry by its id
	/// Get the broadcast entry by its idx

Additional rpcs #535

Are you sure you want to change the base?

Additional rpcs #535

Conversation

purusang commented Dec 12, 2024 • edited by john-light Loading

Description

Type of Change

Notes to Reviewers

Checklist

Related Issues

codecov bot commented Dec 12, 2024 • edited Loading

Codecov Report

delbonis commented Dec 13, 2024

delbonis left a comment

Choose a reason for hiding this comment

bewakes 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

purusang commented Jan 1, 2025 • edited Loading

purusang commented Dec 12, 2024 •

edited by john-light

Loading

codecov bot commented Dec 12, 2024 •

edited

Loading

purusang commented Jan 1, 2025 •

edited

Loading