Initial implementation of outbound message queue, including expiry mechanism #2227
Conversation
There was a problem hiding this comment.
It's a little difficult to evaluate this out of the context of the problem it is attempting to solve. I'd prefer a larger PR that includes the integration code. This is a great data structure and the testing is lovely. Looking forward to seeing it integrated.
core/message_queue.go
Outdated
| // in the queue for that address (indicating the message had already been removed). | ||
| // Returns an error if the expected nonce is greater than the smallest in the queue. | ||
| // The caller may wish to check that the returned message is equal to that expected (not just in nonce value). | ||
| func (mq *MessageQueue) RemoveNext(sender address.Address, expectedNonce uint64) (*types.SignedMessage, error) { |
There was a problem hiding this comment.
It's not clear how the caller is going to know the expected nonce.
In general, there's a lot of logic around nonce and block height tracking that I was expecting to see inside this queue. Will this data structure be wrapped in another, smarter message queue looking thing that handles that logic? It seems a bit much for a plumbing struct.
There was a problem hiding this comment.
The caller is going to see a message in a block and read its nonce from that. Nonces should be contiguous, so a gap will error here. But the sequence may also restart from an earlier point after a reorg.
|
|
||
| // NextStamp returns the stamp for the next message in the queue for an address, or zero if the | ||
| // queue is empty. | ||
| func (mq *MessageQueue) NextStamp(sender address.Address) uint64 { |
There was a problem hiding this comment.
I'll 'fess up that I can't remember, but I think there was a good reason. I should remove this if unused after integration.
|
|
||
| type queuedMessage struct { | ||
| msg *types.SignedMessage | ||
| stamp uint64 |
There was a problem hiding this comment.
Why not just call this blockHeight? That's the intent, and calling it stamp when it's actually a block height will be confusing.
There was a problem hiding this comment.
I hear you and it's worth talking about it. I intend this data structure to be self contained, in that it implemented the interface and semantics described in its comments. While it does have an intended use case, it's correctness need not depend on that.
It wasn't an obvious answer to use block height for the timeout. If we change our mind, nothing in this structure should need to change.
|
@acruikshank I can see we have a difference in philosophy here between this and #2237 (I reviewed that before reading here). I have intentionally decoupled the data structure from the hookup/strategy code that will use it (not yet written) and put it up for review to ensure that it stands alone. The abstraction over the definition of block height, etc, is intentional. There is some chance the interface will need changing as I integrate. I don't yet know what the integration code will be called, but certainly it will be more than just plumbing. A I prefer small PRs that don't do any more than necessary for a self-contained change. I want feedback on the internals here before I do all the hookup work, in case I'm wildly off-target. One alternative approach I could follow (next time) is to do the hookup work but to a stub implementation in one or more PRs, and then fill out the implementation (i.e. top-down). Another alternative is to do all the work in secret and then put up 3-4 PRs, based off each other, for review in parallel. That would give you a lot more context and make it more likely I get the interface exactly right, but I fear the constant chained rebasing involved in such an approach. |
anorth
force-pushed
the
anorth/mqueue
branch
3 times, most recently
from
82e4c01
to
5c05aad
Compare
…chanism. The core of #2146. This is not yet integrated into the message sender plumbing, which will provide values for chain height, or chain syncing, which will remove/expire messages.
This is the core of #2146.
This is not yet integrated into the message sender plumbing, which will provide values for chain height,
or chain syncing, which will remove/expire messages.