-
Notifications
You must be signed in to change notification settings - Fork 295
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
mixing: Add mixpool package. #3082
Conversation
2d0d3c6
to
36b835e
Compare
5c698ec
to
18f2b89
Compare
cfa0c3d
to
c19fc08
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think almost all of this should be an internal package, just like mempool
. Since mixing
is a development module for the time being, it's fine, but ultimately I really don't want to go back to having a huge surface of exposed code that really is specifically for the server itself.
I expect that some of it the code should be shared, akin to how blockchain/standalone
does it, but pretty much everything in mixpool.go
, mixpool_test.go
, and log.go
, at least, should be internal.
Not possible with current design.
This is because the mixclient package expects the wallet interface to provide an accessor function for the mixpool. |
39e3711
to
f056620
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
More to review, but publishing it thus far so you can address it meanwhile.
var ( | ||
// ErrChangeDust is returned by AcceptMessage if a pair request's | ||
// change amount is dust. | ||
ErrChangeDust = errors.New("change output is dust") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is also ignoring all of the best practices in the repo for error handling as mentioned in the PR this one depends on.
However, it's fine for the development module, but it's something we'll want to address.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we don't return these bare anyways, they are always wrapped as a RuleError, but are defined as variables so callers can test for the precise reason with errors.Is. This isn't that much unlike the error code method, but reads nicer.
latestKE: make(map[idPubKey]*wire.MsgMixKeyExchange), | ||
sessions: make(map[[32]byte]*session), | ||
sessionsByTxHash: make(map[chainhash.Hash]*session), | ||
epoch: 10 * time.Minute, // XXX: mainnet epoch: add to chainparams |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not something we need to merge this, but it would probably be nicer to just take the epoch as a part of configuration for the pool rather than having to worry about access to chain params, and needing an exported method to return it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Round 2 review. More coming.
if !mixing.VerifySignedMessage(msg) { | ||
return nil, ruleError(ErrInvalidSignature) | ||
} | ||
id := (*idPubKey)(msg.Pub()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
More panic footgunnage.
sorry, messed up my history and ended up pushing mixclient onto here. squashed all the wip commits down and force pushed just mixpool again. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alright, this looks good to go now. Please squash and rebase it, and I'll get it merged. Nice job overall!
The mixpool package implements a memory pool of recently observed mix messages. Similar to the transaction mempool, the mixpool allows these messages to be temporarily stored in memory to be relayed through the P2P network. It handles message acceptance, expiry, UTXO ownership proof checks, and that previously referenced messages have also been accepted to the mixpool. The mixpool is designed with both full-node and wallet usage in mind, providing all of these same acceptance rules to mixing wallets with the exception of UTXO proof checks. For wallets, it also implements query functions for messages matching compatible pairings and messages belong to ongoing sessions.
The
mixpool
package implements a memory pool of recently observed mix messages. Similar to the transactionmempool
, themixpool
allows these messages to be temporarily stored in memory to be relayed through the P2P network. It handles message acceptance, expiry, UTXO ownership proof checks, and that previously referenced messages have also been accepted to themixpool
.The
mixpool
is designed with both full-node and wallet usage in mind, providing all of these same acceptance rules to mixing wallets with the exception of UTXO proof checks. For wallets, it also implements query functions for messages matching compatible pairings and messages belong to ongoing sessions.Requires #3207.