[compatible] Stop txn/network after slot feature #14516
Conversation
joaosreis
force-pushed
the
joaosreis/hf-stop-txn-after-slot-compatible
branch
from
e54c02f to
f096e48
Compare
joaosreis
changed the title
joaosreis
force-pushed
the
joaosreis/hf-stop-txn-after-slot-compatible
branch
from
d98b47f to
5ac8a89
Compare
integration_tests.mlh (to be reverted later)
joaosreis
force-pushed
the
joaosreis/hf-stop-txn-after-slot-compatible
branch
from
5ac8a89 to
da70971
Compare
|
!ci-nightly-me |
Does this excerpt from RFC mean that there should be no option to set the two parameters via CLI flag? |
|
@georgeee Yes, there should not be a CLI flag. We did align in the RFC discussion to allow a runtime configuration override for integration testing purposes, which can be left undocumented. |
| ok_if_true msg | ||
| ( Mina_numbers.Global_slot.( | ||
| of_uint32 block.slot_since_genesis < slot_tx_end) | ||
| || block.command_transaction_count = 0 |
There was a problem hiding this comment.
NIT: I'd put conjunction group into braces for clarity (not necessary, but required me to remember operator precedence to be sure it works)
There was a problem hiding this comment.
I certainly agree. It just turns out the formatter doesn't let me keep them :)
src/lib/network_pool/indexed_pool.ml
Outdated
| match Map.find t.all_by_sender fee_payer with | ||
| | None -> | ||
| (* nothing queued for this sender *) | ||
| match slot_tx_end with |
There was a problem hiding this comment.
I suggest to flatten this code a little bit.
Something like the following would fit nicely:
let%bind () =
Result.ok_if_true ~error:After_slot_tx_end @@
Option.value_map ~f:Global_slot.(fun slot_tx_end' ->
current_global_slot >= slot_tx_end') ~default:true slot_tx_end in
There was a problem hiding this comment.
TODO for George: re-review the code of the function after flattening (now changes are inflated by increase of layerness)
There was a problem hiding this comment.
Good suggestion. In this case, the slot comparison condition needs to be inverted. Before it was the condition where it would fail, now it should be the condition where it should proceed.
src/lib/network_pool/indexed_pool.ml
Outdated
| (of_time_exn ~constants:consensus_constants | ||
| (Block_time.now time_controller) )) | ||
| in | ||
| match slot_tx_end with |
There was a problem hiding this comment.
Please flatten this code as suggested above
| end | ||
|
|
||
| module Rejected = struct | ||
| [%%versioned | ||
| module Stable = struct | ||
| [@@@no_toplevel_latest_type] | ||
|
|
||
| module V2 = struct |
There was a problem hiding this comment.
Note for future: discuss on how this affects berkeley versioning types (whould we bump some types' versions there too?)
There was a problem hiding this comment.
This is the first time I'm updating versioned types, so I'm unsure on how this would affect
berkeley types.
Regardless, this PR has merge conflicts with berkeley and develop, so we need to merge this feature into there as well. We can then upgrade these types there. Does this solve the issue?
There was a problem hiding this comment.
Yes, for sure. This is the way to proceed. My main point was that if we add a new numbered type here (V2) which existed in different form in berkeley, we probably should make sure to use V3 in berkeley. I.e. if types between compatible and berkeley differ, berkeley should be +1 from compatible. That said, if you will find an existing V3 in berkeley for a type that is V2 in compatible, it's safe to change this type right away without introducing a new number.
There was a problem hiding this comment.
berkeley and develop are expected to have the same types, so once you remake this change against berkeley, there should be no problem with preparing a PR against develop.
| assert_pool_txs independent_cmds' ; | ||
| Deferred.unit ) | ||
|
|
||
| let%test_unit "transactions added after slot_tx_end are rejected" = |
There was a problem hiding this comment.
| let%test_unit "transactions added after slot_tx_end are rejected" = | |
| let%test_unit "transactions added at slot_tx_end are rejected" = |
There was a problem hiding this comment.
Should we add a test case for "after"?
There was a problem hiding this comment.
It surely doesn't hurt.
…op-txn-after-slot-compatible
|
@georgeee please give a look at the changes I just pushed. |
|
!ci-build-me |
|
!ci-build-me |
|
!ci-build-me |
TL;DR implements RFC #14138
This PR introduces two new optional compile-time configuration parameters,
slot_tx_endandslot_chain_end. These define, respectively, the slot at which the node will stop producing/validating non-empty blocks (defined as being blocks that produce an empty staged ledger diff), and the slop at which the node stops producing/accepting any blocks at all. This is a feature part of the hard-fork mechanism.Summary of changes:
slot-tx-endparameter to CLI.slot-tx-endslot instead of fetching them from the transaction pool.slot-tx-endslot.slot-tx-endslot.slot-tx-endslot is reached.Some unit tests (specifically, for the transaction pool) were added. The complete set of expected behaviors was demonstrated in a sandbox network.
Merge PRs to other branches: