Exp 191 refactor inscription manager #249
Conversation
bewakes
force-pushed
the
EXP-191-refactor-inscription-manager
branch
4 times, most recently
from
427ed2d
to
6ffd0ad
Compare
bewakes
force-pushed
the
EXP-191-refactor-inscription-manager
branch
from
1505ad5
to
294434d
Compare
bewakes
force-pushed
the
EXP-191-refactor-inscription-manager
branch
from
294434d
to
0f46f55
Compare
Codecov ReportAttention: Patch coverage is
@@ Coverage Diff @@
## master #249 +/- ##
==========================================
+ Coverage 57.90% 58.71% +0.81%
==========================================
Files 119 123 +4
Lines 13425 13406 -19
==========================================
+ Hits 7774 7872 +98
+ Misses 5651 5534 -117
|
bewakes
force-pushed
the
EXP-191-refactor-inscription-manager
branch
from
0f46f55
to
2b4b502
Compare
bewakes
force-pushed
the
EXP-191-refactor-inscription-manager
branch
from
2b4b502
to
910d1bb
Compare
bewakes
force-pushed
the
EXP-191-refactor-inscription-manager
branch
from
910d1bb
to
1cb8fbc
Compare
There was a problem hiding this comment.
The inscription worker similarly to the earlier version of the broadcaster worker relies on polling the database to be notified of new work. This adds latency and io overhead to the system, so it should instead use a channel to wake up the inscription worker when there's new work to do. It should still timeout and poll to check to see if any of the txs it was waiting on got finalized or anything (also eventually we'd want this to be done based on a wakeup from the L1 reader task) and update whatever it needs to in the database, which means we can use a much slower poll rate without compromising latency.
| ) -> anyhow::Result<(Buf32, Buf32)> { | ||
| let (commit, reveal) = build_inscription_txs(&blobentry.blob, &client, config).await?; | ||
|
|
||
| debug!("Signing commit transaction {}", commit.compute_txid()); |
There was a problem hiding this comment.
Should use a param like:
let commit_txid = commit.compute_txid();
debug!(%commit_txid, "signing inscription commit tx");so that we can search the logs more easily
There was a problem hiding this comment.
Added the changes in this commit: 77ed0fa
| warn!("Received intent not meant for L1: {}", intent.commitment()); | ||
| return Ok(()); | ||
| } | ||
|
|
||
| let entry = BlobEntry::new_unsigned(intent.payload().to_vec()); | ||
| debug!("Received intent: {}", intent.commitment()); | ||
| if self | ||
| .ops | ||
| .get_blob_entry_blocking(*intent.commitment())? | ||
| .is_some() | ||
| { | ||
| warn!("Received duplicate intent {:?}", intent.commitment()); |
There was a problem hiding this comment.
Should all use param logs instead of making it part of the message.
| .ops | ||
| .put_blob_entry_async(*intent.commitment(), entry) | ||
| .await?) | ||
| } |
There was a problem hiding this comment.
Same things about the params here.
| l1_status, | ||
| ) | ||
| .await | ||
| .unwrap() |
There was a problem hiding this comment.
Do we want to unwrap this error here? I might not remember the specifics about how the task manager should be used but don't we want to bubble the error types up to it to let it do the error handling instead of panicking and not being able to pick up the structured errors?
| loop { | ||
| interval.as_mut().tick().await; | ||
|
|
||
| if let Some(blobentry) = insc_ops.get_blob_entry_by_idx_async(curr_blobidx).await? { |
There was a problem hiding this comment.
This is still relying on polling the database instead of being woken up by a message on a channel in the writer handle indicating that there's new blobs to sign, right?
| pub struct InscriptionHandle { | ||
| ops: Arc<InscriptionDataOps>, | ||
| } | ||
|
|
There was a problem hiding this comment.
Needs some way to signal to the writer task that there's new blobs to sign instead of waiting on the task to poll the database.
Also should be called InscriberHandle or InscriptionWorkerHandle or something.
There was a problem hiding this comment.
Since this polling will not be that frequent as we are posting inscriptions every 10 mins or so, polling db didn't seem that concerning and is much simpler. The broadcaster does have the signaling because it needs to broadcast frequently as txs might come from L1Writer or from the bridge operator.
| // NOTE: It would be nice to return reveal txid from the submit method. But creation of txs | ||
| // is deferred to signer in the writer module |
There was a problem hiding this comment.
We could return the blob index here.
Description
This is a refatoring of inscription/writer task.
Type of Change
Checklist
Related Issues