Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: high priority processing of economic activity #7483

Merged
merged 5 commits into from
Apr 27, 2023

Conversation

mhofman
Copy link
Member

@mhofman mhofman commented Apr 23, 2023

closes: #5966
closes: #6576
closes: #6964
refs: #5334

Best reviewed commit-by-commit

Description

This change introduces a high priority queue, with cosmic swingset processing actions from that queue before timer polls (#6964) and non-priority actions (#5966).

It also introduces a new chain storage prefix highPrioritySenders where it expects to find entries for each address to be considered for priority processing. This path is top level and will require a new a new "root" chain storage created for it in bootstrap. JS should populate that collection by creating a child node for the address, and setting any non-empty value. To remove the address, it should set the node to the empty string.

When considering messages, the inbound ante handler will check whether the message is from a high priority sender, and allow the message regardless of the inbound queue size.

Security Considerations

This introduces a way to bypass inbound queue size checks, but gated on the verified signer.

Scaling Considerations

The lookup of the high priority status of the sender must be done for every message going into the mempool, and is thus done in O(1) by a direct path lookup.

Documentation Considerations

TBD

Testing Considerations

Added a unit test for the inbound checks in golang
We do not have tests covering the routing of messages, either on the golang or the JS side, and I didn't add any.
In a perfect world we should have an integration test for #5334 verifying the end-to-end behavior.

@mhofman mhofman requested review from gibson042 and JimLarson April 23, 2023 01:19
@mhofman
Copy link
Member Author

mhofman commented Apr 23, 2023

cc @turadg @dckc

@mhofman mhofman force-pushed the mhofman/5334-economic-prioritization branch from 6256501 to 07f7a33 Compare April 23, 2023 20:08
Comment on lines 15 to 22
This AnteDecorator enforces a limit on the size of the Swingset inbound queue
by scanning for Cosmos-messages which add Swingset-messages to the queue. Note
that when running DeliverTx, inbound messages are placed in either the
actionQueue or the priorityQueue (together forming the Swingset inbound queue),
and kept there until processed by SwingSet. Previous Txs in the block will have
already been added to the inbound queue, so we must reject Txs which would grow
the actionQueue beyond the allowed inbound size. The priorityQueue is exempt
of size checks.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please don't mix reformatting with substantial changes in a single commit - separate reformatting and other whitespace-only changes, and maybe mechanical renaming, into a separate commit so that the reviewer is free to skim it.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I took a shortcut and described the about to be semantics. I'll try to update the comment to first reflect the naming change before updating again when changing the semantics.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let me know if the new commit-by-commit change addresses your concern.

golang/cosmos/ante/inbound.go Outdated Show resolved Hide resolved
golang/cosmos/ante/inbound.go Outdated Show resolved Hide resolved
@JimLarson
Copy link
Contributor

Note: "Priority Queue" is a data structure which lets you remove the highest-priority item efficiently, so calling the high-priority queue the "priority queue" is potentially confusing. Consider calling it the "high priority queue", which leaves room for additional priority levels in the future. Avoid calling it the "economy queue" as a) it mixes mechanism with policy, and b) suggests a lower priority by analogy with airline boarding.

@mhofman mhofman removed the request for review from gibson042 April 24, 2023 00:28
@mhofman mhofman force-pushed the mhofman/5334-economic-prioritization branch from 07f7a33 to aaefa79 Compare April 24, 2023 15:36
@mhofman mhofman changed the title feat: priority processing of economic activity feat: high priority processing of economic activity Apr 24, 2023
@mhofman mhofman marked this pull request as ready for review April 24, 2023 15:38
@mhofman mhofman requested review from JimLarson and gibson042 April 24, 2023 15:38
@mhofman mhofman force-pushed the mhofman/5334-economic-prioritization branch from aaefa79 to 85f0c7a Compare April 24, 2023 15:46
Copy link
Contributor

@JimLarson JimLarson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with one minor issue to consider.

golang/cosmos/x/swingset/types/msgs.go Outdated Show resolved Hide resolved
Comment on lines 19 to 22
and kept there until processed by SwingSet. Previous Txs in the block will have
already been added to the inbound queue, so we must reject Txs which would grow
the actionQueue beyond the allowed inbound size. The highPriorityQueue is
exempt of size checks but still consume allotments.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What block is "the block" here (e.g., is it presumptive next block still under construction)? And what are the "allotments"?

return k.pushAction(ctx, StoragePathHighPriorityQueue, action)
}

func (k Keeper) queueIndex(ctx sdk.Context, queuePath string, position string) (sdk.Int, error) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: Move the string concatenation into the caller (where it already exists anyway in the case of pushAction) and refactor into something like

Suggested change
func (k Keeper) queueIndex(ctx sdk.Context, queuePath string, position string) (sdk.Int, error) {
func (k Keeper) getInt(ctx sdk.Context, path string, default *sdk.Int) (sdk.Int, error) {
 	// Get the current queue tail, defaulting to zero if its vstorage doesn't exist.
 	// The `tail` is the value of the next index to be inserted
-	tail, err := k.queueIndex(ctx, inboundQueuePath, "tail")
+	tail, err := k.getInt(ctx, inboundQueuePath + ".tail", zero)
 	if err != nil {
 		return err
 	}
 func (k Keeper) queueLength(ctx sdk.Context, queuePath string) (sdk.Int, error) {
-	head, err := k.queueIndex(ctx, queuePath, "head")
+	head, err := k.getInt(ctx, queuePath + ".head", zero)
 	if err != nil {
 		return sdk.NewInt(0), err
 	}
-	tail, err := k.queueIndex(ctx, queuePath, "tail")
+	tail, err := k.getInt(ctx, queuePath + ".tail", zero)
 	if err != nil {
 		return sdk.NewInt(0), err
 	}
 }

It could even be moved down into vstorage/keeper, and/or the queue concept could be abstracted here into e.g. queuePush(ctx sdk.Context, queuePath string) error and queueLength(ctx sdk.Context, queuePath string) (sdk.Int, error) rather than the mismatched pushAction and queueLength.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Refactored and moved to vstorage mostly as suggested (minus the default value).

Comment on lines 266 to 279
const queueRawStorage = makeBufferedStorage(
makePrefixedBridgeStorage(
sendToChain,
`${queuePath}.`,
'setWithoutNotify',
x => x,
x => x,
),
);
return harden({
...queueRawStorage.kvStore,
commit: queueRawStorage.commit,
abort: queueRawStorage.abort,
});
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there's a slightly more conventional way to express this.

Suggested change
const queueRawStorage = makeBufferedStorage(
makePrefixedBridgeStorage(
sendToChain,
`${queuePath}.`,
'setWithoutNotify',
x => x,
x => x,
),
);
return harden({
...queueRawStorage.kvStore,
commit: queueRawStorage.commit,
abort: queueRawStorage.abort,
});
const { kvStore, commit, abort } = makeBufferedStorage(
makePrefixedBridgeStorage(
sendToChain,
`${queuePath}.`,
'setWithoutNotify',
x => x,
x => x,
),
);
return harden({ ...kvStore, commit, abort });

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Duh

Comment on lines 50 to 53
* golang side will push into the queue, updating `<prefix>tail` and setting
* `<prefix><index>`.
* The JS side will shift the queue, updating `<prefix>head` and reading and
* deleting `<prefix><index>`.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* golang side will push into the queue, updating `<prefix>tail` and setting
* `<prefix><index>`.
* The JS side will shift the queue, updating `<prefix>head` and reading and
* deleting `<prefix><index>`.
* golang side will push into the queue, updating the index stored at key
* `${queuePath}.tail` and setting data for key `${queuePath}.${index}`.
* The JS side will shift the queue, updating the index at key
* `${queuePath}.head` and reading and deleting `${queuePath}.${index}`.

@mhofman mhofman added the automerge:rebase Automatically rebase updates, then merge label Apr 26, 2023
@mhofman mhofman force-pushed the mhofman/5334-economic-prioritization branch from b8d47e3 to ca631c1 Compare April 27, 2023 04:10
@mhofman
Copy link
Member Author

mhofman commented Apr 27, 2023

@Mergifyio refresh

@mergify
Copy link
Contributor

mergify bot commented Apr 27, 2023

refresh

✅ Pull request refreshed

mhofman added 5 commits April 27, 2023 05:34
The inbound queue represents the combination of queues with actions
destined to SwingSet, currently only the `actionQueue`.
Rename things appropriately.
Drain the `highPriorityQueue` before the `actionQueue`
(#5966) and before polling
timers (#6964).
Based on the presence of their address in a given vstorage path
@mhofman mhofman force-pushed the mhofman/5334-economic-prioritization branch from ca631c1 to 740840f Compare April 27, 2023 05:34
@mergify mergify bot merged commit e056a61 into master Apr 27, 2023
@mergify mergify bot deleted the mhofman/5334-economic-prioritization branch April 27, 2023 06:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge:rebase Automatically rebase updates, then merge
Projects
None yet
3 participants