blockprod: Ability to specify custom transactions #1270
Conversation
iljakuklic
added
enhancement
iljakuklic
requested review from
alfiedotwtf,
OBorce,
TheQuantumPhysicist and
azarovh
iljakuklic
force-pushed
the
blockprod-custom-txs
branch
from
fef6500
to
45d773a
Compare
| // TODO: Alternatively, we can construct the transaction sequence from | ||
| // scratch every time a different timestamp is attempted. That is more costly | ||
| // in terms of computational resources but will allow the node to include more | ||
| // transactions since the passing time may release some time locks. |
There was a problem hiding this comment.
Going through the above code and then this change, I'm thinking that the minimum here should be MIN(last_timestamp_seconds_used, min_constructed_block_timestamp) given that transactions may be earlier than min_constructed_block_timestamp.
min_contstructed_block_timestamp should probably by locally scoped to the construction of max_constructed_block_timestamp as it's only used to calculate that value.
mempool/src/pool/collet_txs.rs
Outdated
| // Set of transactions already placed into the accumulator | ||
| let mut emitted: BTreeSet<_> = accum_ids.iter().collect(); | ||
| // Set of already processed transactions, for de-duplication | ||
| let mut processed = emitted.clone(); |
|
Race condition fix in 22940c6 looks good |
|
Apparently, the mempool sync issue resurfaced also in functional tests, need to fix it up. |
iljakuklic
force-pushed
the
blockprod-custom-txs
branch
from
8dc465d
to
f533dde
Compare
| accumulator | ||
| .add_tx(transaction, Amount::ZERO.into()) | ||
| .map_err(|err| BlockProductionError::FailedToAddTransaction(transaction_id, err))? |
There was a problem hiding this comment.
How are we making sure that these raw transactions are valid against what's coming from the mempool? I do hope we're not depending on the temporary solution we deployed that we wanted to solve later, where we include a transaction verifier inside the accumulator.
There was a problem hiding this comment.
There is no guarantee currently. Even transaction verifier is not sufficient because mempool deps are used to figure out the parent/child relationships. So it only properly works for transactions that are "independent" of transactions in the mempool. Could be removed since the guarantees are much weaker than for transactions specified by IDs.
There was a problem hiding this comment.
I thought we agreed to just remove it...
There was a problem hiding this comment.
I thought we agreed to just remove it...
Ok will do.
There was a problem hiding this comment.
Hmm there are many existing functional tests that rely on this functionality, and some are not easy to adapt so the transactions can be pushed through mempool.
There was a problem hiding this comment.
Let's just keep it, and write in the documentation that adding transactions there relies on the user ensuring the validity, and is done only in testing.
|
I took a round and it looks fine. I just don't understand the consistency guarantees we're having on transactions. Besides that and the comments, looking good. |
iljakuklic
force-pushed
the
blockprod-custom-txs
branch
2 times, most recently
from
3426273
to
2080d38
Compare
This gives the user the option to add a custom list of transactions when asking the node to generate a new block. Based on the original work of @alfiedotwtf.
iljakuklic
force-pushed
the
blockprod-custom-txs
branch
from
2080d38
to
78e42ea
Compare
iljakuklic
force-pushed
the
blockprod-custom-txs
branch
from
78e42ea
to
3a3ed64
Compare
This gives the user the option to add a custom list of transactions when asking the node to generate a new block.
Based on the original work of @alfiedotwtf.
Related: