change tombstone block default to 10 #246
Conversation
| @@ -641,8 +641,7 @@ where | |||
| transaction_builder.set_fee(Mob::MINIMUM_FEE)?; | |||
|
|
|||
| let num_blocks_in_ledger = self.ledger_db.num_blocks()?; | |||
| transaction_builder.set_tombstone_block(num_blocks_in_ledger + 50); | |||
|
|
|||
| transaction_builder.set_tombstone_block(num_blocks_in_ledger + 10); | |||
There was a problem hiding this comment.
I'm totally new to rust and FS, but it seems to me like this value of 10 here should come from the constant DEFAULT_NEW_TX_BLOCK_ATTEMPTS rather than be hard coded?
There was a problem hiding this comment.
Very happy to import it from the transaction_builder service. Is it best left as a public constant off of that service, or should we move the const to an external constants provider (the mobilecoin repo's constants.rs file or similar)?
There was a problem hiding this comment.
It looks like there's the TODO for making this value configurable. Maybe once we implement that it will make sense to move this to a config file, but until then I think it makes sense to import it from transaction_builder.rs.
There was a problem hiding this comment.
Maybe update the documentation for use that variable name instead of the number 10? that way if we tweak it again we don't have to update all the documentation as well?
There was a problem hiding this comment.
Currently I don't feel good about directing folks to look into the transaction_builder for this value when they're looking at the documentation. When we move this constant to a more unified location then I think we should update the docs to point to the constant at that point, but we should do that along with other constants that have been pulled in. Navigating docs should require minimal lift on the readers behalf.
|
I see there is a linting failure, but my local linting passes, and the diff listed in circleCI doesn't show anything I can see. Any idea what might be causing it? https://app.circleci.com/pipelines/github/mobilecoinofficial/full-service/1295/workflows/0a31f10f-421f-4778-9879-27401368fbb4/jobs/3468/parallel-runs/0/steps/0-114 |
|
The |
* make validator failure logs more verbose to ease debugging (#239) * Fix a bug in receipt status endpoint. (#233) * Uprev MobileCoin Library (#242) * Update TXO documentation (#241) * Update builder and base image references (#244) * Feature/sync performance (#245) * change tombstone block default to 10 (#246) * adding max limit to some API endpoints (#248)
Motivation
Currently the tombstone block default is set to 50, but this is creating a too-large window of transaction validity and we're looking to shorten that.
In this PR
updates the default tombstone block count from 50 to 10
updates tests to refer to generated block counts 40 lower than before (the difference between the new and old tombstoning values)
fixes ticket https://app.asana.com/0/1200175610892874/1200901840461971/f
Future Work
This work was done in part because the current transaction rate is low enough that transactions are being kept valid for too long with a default tombstone block count of 50. In the future, the scale of this will likely change. It should be revisited when we have scaled the system, as the problem may reverse.