Draft: Refactor channels with generic jobs #2030
Conversation
coleFD
force-pushed
the
refactor-channels-with-generic-jobs
branch
from
59e98c7 to
89dd956
Compare
|
|
||
| /// Returns an owned copy of a stale job from its job ID, if any. | ||
| fn get_stale_job(&self, job_id: u32) -> Option<T>; | ||
| fn try_validate_job( |
There was a problem hiding this comment.
I went this route because the current trait kinda nudges you toward having the same structure as DefaultJobStore. In it's most basic form I beleive the JobStore should allow you to:
- add an active job
- add and activate a future job
- make jobs stale
- retrieve jobs for validation
The extra methods are bloat imo.
In our case we use timestamp based job ids, and it could be possible---albeit a very low chance---that we have duplicate job_ids. Also, assuming there is access to the underlying JobStore, the user could override share validation to allow for merged mining
| @@ -1,6 +1,6 @@ | |||
| [package] | |||
| name = "channels_sv2" | |||
| version = "3.0.0" | |||
There was a problem hiding this comment.
version 3.0.0 hasn't been published yet, so no need to bump this
see #2010 (comment)
coleFD
force-pushed
the
refactor-channels-with-generic-jobs
branch
from
cceca4a to
9d82e95
Compare
| /// | ||
| /// Enables creation of new Standard Jobs from NewTemplate messages. | ||
| /// | ||
| /// The `JobContainer` generic parameter allows for smart pointers that wrap `<dyn Job<'a>>` (ie. `Box<dyn Job<'a>>`, `Arc<dyn Job<'a>>`, etc.) |
There was a problem hiding this comment.
outdated comment. Will remove
coleFD
force-pushed
the
refactor-channels-with-generic-jobs
branch
from
bfa271a to
420de90
Compare
| /// That makes it easy to calculate the coinbase `txid` (instead of `wtxid`) for merkle root | ||
| /// calculation. | ||
| #[derive(Debug, Clone, PartialEq)] | ||
| pub struct EitherJob<'a> { |
There was a problem hiding this comment.
so why are we going with a concrete type EitherJob?
from the description of #2019 and discussions on #2011 (comment), I was more under the impression that we were going to introduce new traits for Extended and Standard jobs?
There was a problem hiding this comment.
I started down that route but it was painful. I realized the only difference between the jobs once they are stored in the channel is a pre-calcuated merkle root so it's easier to just make a single job struct with an optional merkle_root field
There was a problem hiding this comment.
Granted, JobStore does still allow Generic jobs that dont have to be EitherJob. We will need to implement our own jobs for merged mining so this is still possible. The EitherJob just makes compatibility easier with this set up
There was a problem hiding this comment.
but it was painful.
can you elaborate? what kinds of challenges did you encounter?
There was a problem hiding this comment.
Casting from the strong typed result of the JobFactory::new_extended_job or new_standard_job to the trait. Sure we could box the types, but EitherJob seems more elegant to me considering the only difference is a cached merkle_root. I even noticed coinbase_prefix and suffix are calculated for the original calculation of the merkle_root for standard jobs in the JobFactory and then they just get thrown away, which requires rebuilding and serializing the coinbase on every share considering ShareValidationResult::BlockFound requires a serialized coinbase transaction. Seems like a ways to not keep the prefix/suffix and do the same thing as an extended share
| JobIn: Job<'a> + From<EitherJob<'a>> + Clone, | ||
| JobOut: Job<'a> + Clone, |
There was a problem hiding this comment.
what's the rationale behind JobIn vs JobOut?
There was a problem hiding this comment.
future jobs need to be mutable to activate min_ntime, active jobs are immutable. So you can add a future job of type T and then your implementation can wrap that as Arc<T> or Rc<T> if you want when fetching jobs to avoid cloning the full job
2 participants
No description provided.