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

wire: Add p2p mixing messages. #3066

Merged
merged 1 commit into from
Mar 19, 2024
Merged

wire: Add p2p mixing messages. #3066

merged 1 commit into from
Mar 19, 2024

Conversation

jrick
Copy link
Member

@jrick jrick commented Feb 27, 2023

These messages implement the stages of a cspp mix. Messages will be broadcast to all full nodes and all peers participating in mixing through inventory messages.

Copy link
Member

@davecgh davecgh left a comment

Choose a reason for hiding this comment

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

Overall this is looking pretty good so far. I still have a fair bit of review to do as I'm only about half way through, but I figured I'd go ahead and push the first portion of the review so some things can be addressed in the mean time.

wire/msgmixsr_test.go Outdated Show resolved Hide resolved
wire/common.go Outdated Show resolved Hide resolved
wire/msgmixcm.go Outdated Show resolved Hide resolved
wire/msgmixcm_test.go Outdated Show resolved Hide resolved
wire/mixvec.go Outdated Show resolved Hide resolved
wire/msgmixcm.go Outdated Show resolved Hide resolved
wire/msgmixcm.go Outdated Show resolved Hide resolved
wire/msgmixcm.go Outdated Show resolved Hide resolved
wire/msgmixcm.go Outdated Show resolved Hide resolved
"github.com/decred/dcrd/chaincfg/chainhash"
)

func TestMixCMWire(t *testing.T) {
Copy link
Member

Choose a reason for hiding this comment

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

These tests have pretty poor test coverage:

$ go test -coverprofile=cov.out >/dev/null && go tool cover -func=cov.out | grep MixCM
github.com/decred/dcrd/wire/msgmixcm.go:205:            NewMsgMixCM             66.7%

Copy link
Member Author

@jrick jrick Jan 12, 2024

Choose a reason for hiding this comment

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

Current test coverage percents:

$ go test -coverprofile=cov.out >/dev/null && go tool cover -func=cov.out | grep msgmix
github.com/decred/dcrd/wire/msgmixciphertexts.go:35:	BtcDecode		76.9%
github.com/decred/dcrd/wire/msgmixciphertexts.go:83:	BtcEncode		63.6%
github.com/decred/dcrd/wire/msgmixciphertexts.go:109:	Hash			0.0%
github.com/decred/dcrd/wire/msgmixciphertexts.go:119:	WriteHash		0.0%
github.com/decred/dcrd/wire/msgmixciphertexts.go:136:	writeMessageNoSignature	65.2%
github.com/decred/dcrd/wire/msgmixciphertexts.go:181:	WriteSignedData		0.0%
github.com/decred/dcrd/wire/msgmixciphertexts.go:188:	Command			100.0%
github.com/decred/dcrd/wire/msgmixciphertexts.go:194:	MaxPayloadLength	100.0%
github.com/decred/dcrd/wire/msgmixciphertexts.go:204:	Pub			0.0%
github.com/decred/dcrd/wire/msgmixciphertexts.go:209:	Sig			0.0%
github.com/decred/dcrd/wire/msgmixciphertexts.go:214:	Expires			0.0%
github.com/decred/dcrd/wire/msgmixciphertexts.go:219:	PrevMsgs		0.0%
github.com/decred/dcrd/wire/msgmixciphertexts.go:224:	Sid			0.0%
github.com/decred/dcrd/wire/msgmixciphertexts.go:229:	GetRun			0.0%
github.com/decred/dcrd/wire/msgmixciphertexts.go:236:	NewMsgMixCiphertexts	100.0%
github.com/decred/dcrd/wire/msgmixconfirm.go:38:	BtcDecode		73.9%
github.com/decred/dcrd/wire/msgmixconfirm.go:81:	BtcEncode		63.6%
github.com/decred/dcrd/wire/msgmixconfirm.go:107:	Hash			0.0%
github.com/decred/dcrd/wire/msgmixconfirm.go:117:	WriteHash		0.0%
github.com/decred/dcrd/wire/msgmixconfirm.go:134:	writeMessageNoSignature	68.4%
github.com/decred/dcrd/wire/msgmixconfirm.go:172:	WriteSignedData		0.0%
github.com/decred/dcrd/wire/msgmixconfirm.go:179:	Command			100.0%
github.com/decred/dcrd/wire/msgmixconfirm.go:185:	MaxPayloadLength	100.0%
github.com/decred/dcrd/wire/msgmixconfirm.go:195:	Pub			0.0%
github.com/decred/dcrd/wire/msgmixconfirm.go:200:	Sig			0.0%
github.com/decred/dcrd/wire/msgmixconfirm.go:205:	Expires			0.0%
github.com/decred/dcrd/wire/msgmixconfirm.go:210:	PrevMsgs		0.0%
github.com/decred/dcrd/wire/msgmixconfirm.go:215:	Sid			0.0%
github.com/decred/dcrd/wire/msgmixconfirm.go:220:	GetRun			0.0%
github.com/decred/dcrd/wire/msgmixconfirm.go:227:	NewMsgMixConfirm	66.7%
github.com/decred/dcrd/wire/msgmixdcnet.go:33:		BtcDecode		76.0%
github.com/decred/dcrd/wire/msgmixdcnet.go:78:		BtcEncode		63.6%
github.com/decred/dcrd/wire/msgmixdcnet.go:104:		Hash			0.0%
github.com/decred/dcrd/wire/msgmixdcnet.go:114:		WriteHash		0.0%
github.com/decred/dcrd/wire/msgmixdcnet.go:131:		writeMessageNoSignature	61.5%
github.com/decred/dcrd/wire/msgmixdcnet.go:175:		writeMixVects		70.6%
github.com/decred/dcrd/wire/msgmixdcnet.go:206:		readMixVects		64.0%
github.com/decred/dcrd/wire/msgmixdcnet.go:252:		WriteSignedData		0.0%
github.com/decred/dcrd/wire/msgmixdcnet.go:259:		Command			100.0%
github.com/decred/dcrd/wire/msgmixdcnet.go:265:		MaxPayloadLength	100.0%
github.com/decred/dcrd/wire/msgmixdcnet.go:275:		Pub			0.0%
github.com/decred/dcrd/wire/msgmixdcnet.go:280:		Sig			0.0%
github.com/decred/dcrd/wire/msgmixdcnet.go:285:		Expires			0.0%
github.com/decred/dcrd/wire/msgmixdcnet.go:290:		PrevMsgs		0.0%
github.com/decred/dcrd/wire/msgmixdcnet.go:295:		Sid			0.0%
github.com/decred/dcrd/wire/msgmixdcnet.go:300:		GetRun			0.0%
github.com/decred/dcrd/wire/msgmixdcnet.go:307:		NewMsgMixDCNet		100.0%
github.com/decred/dcrd/wire/msgmixkeyexchange.go:48:	BtcDecode		75.0%
github.com/decred/dcrd/wire/msgmixkeyexchange.go:86:	BtcEncode		63.6%
github.com/decred/dcrd/wire/msgmixkeyexchange.go:112:	Hash			0.0%
github.com/decred/dcrd/wire/msgmixkeyexchange.go:122:	WriteHash		0.0%
github.com/decred/dcrd/wire/msgmixkeyexchange.go:139:	writeMessageNoSignature	68.8%
github.com/decred/dcrd/wire/msgmixkeyexchange.go:173:	WriteSignedData		0.0%
github.com/decred/dcrd/wire/msgmixkeyexchange.go:180:	Command			100.0%
github.com/decred/dcrd/wire/msgmixkeyexchange.go:186:	MaxPayloadLength	100.0%
github.com/decred/dcrd/wire/msgmixkeyexchange.go:196:	Pub			0.0%
github.com/decred/dcrd/wire/msgmixkeyexchange.go:201:	Sig			0.0%
github.com/decred/dcrd/wire/msgmixkeyexchange.go:206:	Expires			0.0%
github.com/decred/dcrd/wire/msgmixkeyexchange.go:211:	PrevMsgs		0.0%
github.com/decred/dcrd/wire/msgmixkeyexchange.go:216:	Sid			0.0%
github.com/decred/dcrd/wire/msgmixkeyexchange.go:221:	GetRun			0.0%
github.com/decred/dcrd/wire/msgmixkeyexchange.go:228:	NewMsgMixKeyExchange	100.0%
github.com/decred/dcrd/wire/msgmixpairreq.go:77:	Pairing			0.0%
github.com/decred/dcrd/wire/msgmixpairreq.go:105:	BtcDecode		69.0%
github.com/decred/dcrd/wire/msgmixpairreq.go:206:	BtcEncode		63.6%
github.com/decred/dcrd/wire/msgmixpairreq.go:232:	Hash			0.0%
github.com/decred/dcrd/wire/msgmixpairreq.go:242:	WriteHash		0.0%
github.com/decred/dcrd/wire/msgmixpairreq.go:259:	writeMessageNoSignature	62.1%
github.com/decred/dcrd/wire/msgmixpairreq.go:363:	WriteSignedData		0.0%
github.com/decred/dcrd/wire/msgmixpairreq.go:370:	Command			100.0%
github.com/decred/dcrd/wire/msgmixpairreq.go:376:	MaxPayloadLength	100.0%
github.com/decred/dcrd/wire/msgmixpairreq.go:386:	Pub			0.0%
github.com/decred/dcrd/wire/msgmixpairreq.go:391:	Sig			0.0%
github.com/decred/dcrd/wire/msgmixpairreq.go:396:	Expires			0.0%
github.com/decred/dcrd/wire/msgmixpairreq.go:401:	PrevMsgs		0.0%
github.com/decred/dcrd/wire/msgmixpairreq.go:406:	Sid			0.0%
github.com/decred/dcrd/wire/msgmixpairreq.go:411:	GetRun			0.0%
github.com/decred/dcrd/wire/msgmixpairreq.go:418:	NewMsgMixPairReq	100.0%
github.com/decred/dcrd/wire/msgmixsecrets.go:36:	writeMixVect		64.3%
github.com/decred/dcrd/wire/msgmixsecrets.go:63:	readMixVect		66.7%
github.com/decred/dcrd/wire/msgmixsecrets.go:102:	BtcDecode		71.4%
github.com/decred/dcrd/wire/msgmixsecrets.go:152:	BtcEncode		63.6%
github.com/decred/dcrd/wire/msgmixsecrets.go:178:	Hash			0.0%
github.com/decred/dcrd/wire/msgmixsecrets.go:188:	WriteHash		0.0%
github.com/decred/dcrd/wire/msgmixsecrets.go:204:	writeMessageNoSignature	71.4%
github.com/decred/dcrd/wire/msgmixsecrets.go:233:	WriteSignedData		0.0%
github.com/decred/dcrd/wire/msgmixsecrets.go:240:	Command			100.0%
github.com/decred/dcrd/wire/msgmixsecrets.go:246:	MaxPayloadLength	100.0%
github.com/decred/dcrd/wire/msgmixsecrets.go:256:	Pub			0.0%
github.com/decred/dcrd/wire/msgmixsecrets.go:261:	Sig			0.0%
github.com/decred/dcrd/wire/msgmixsecrets.go:266:	Expires			0.0%
github.com/decred/dcrd/wire/msgmixsecrets.go:275:	PrevMsgs		0.0%
github.com/decred/dcrd/wire/msgmixsecrets.go:280:	Sid			0.0%
github.com/decred/dcrd/wire/msgmixsecrets.go:285:	GetRun			0.0%
github.com/decred/dcrd/wire/msgmixsecrets.go:292:	NewMsgMixSecrets	100.0%
github.com/decred/dcrd/wire/msgmixslotreserve.go:47:	BtcDecode		66.0%
github.com/decred/dcrd/wire/msgmixslotreserve.go:125:	BtcEncode		63.6%
github.com/decred/dcrd/wire/msgmixslotreserve.go:151:	Hash			0.0%
github.com/decred/dcrd/wire/msgmixslotreserve.go:161:	WriteHash		0.0%
github.com/decred/dcrd/wire/msgmixslotreserve.go:178:	writeMessageNoSignature	58.3%
github.com/decred/dcrd/wire/msgmixslotreserve.go:256:	WriteSignedData		0.0%
github.com/decred/dcrd/wire/msgmixslotreserve.go:263:	Command			100.0%
github.com/decred/dcrd/wire/msgmixslotreserve.go:269:	MaxPayloadLength	100.0%
github.com/decred/dcrd/wire/msgmixslotreserve.go:279:	Pub			0.0%
github.com/decred/dcrd/wire/msgmixslotreserve.go:284:	Sig			0.0%
github.com/decred/dcrd/wire/msgmixslotreserve.go:289:	Expires			0.0%
github.com/decred/dcrd/wire/msgmixslotreserve.go:294:	PrevMsgs		0.0%
github.com/decred/dcrd/wire/msgmixslotreserve.go:299:	Sid			0.0%
github.com/decred/dcrd/wire/msgmixslotreserve.go:304:	GetRun			0.0%
github.com/decred/dcrd/wire/msgmixslotreserve.go:311:	NewMsgMixSlotReserve	100.0%

It's still missing hitting the error paths, which would probably require crafting messages to exceed the maximum constants.

@jrick jrick force-pushed the wire-mix branch 4 times, most recently from 6693718 to 6dd9ce6 Compare November 21, 2023 20:58
Copy link
Member

@davecgh davecgh left a comment

Choose a reason for hiding this comment

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

This looks really good overall. I have a bunch of comments, but many of them are repeats of the same things that I made mainly for tracking purposes to make it easier to ensure they're all addressed.

wire/error.go Show resolved Hide resolved
wire/error_test.go Show resolved Hide resolved
wire/msgmixkeyexchange.go Outdated Show resolved Hide resolved
wire/msgmixciphertexts.go Show resolved Hide resolved
wire/msgmixciphertexts.go Outdated Show resolved Hide resolved
wire/msgmixpairreq.go Outdated Show resolved Hide resolved
wire/msgmixpairreq.go Show resolved Hide resolved
wire/msgmixpairreq.go Outdated Show resolved Hide resolved
wire/msgmixpairreq.go Show resolved Hide resolved
wire/msgmixpairreq.go Outdated Show resolved Hide resolved
Copy link
Member

@davecgh davecgh left a comment

Choose a reason for hiding this comment

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

Alright, everything has been quite thoroughly reviewed including attempts to intentionally send bogus data that will break things as well as subjecting it to fuzzing.

The only full bug I hit is the one for the size checks for the kpcount that I identified in inline.

Super nice job overall on this. Once everything is addressed, I'll give it another pass and it should be ready to go in.

wire/msgmixpairreq.go Outdated Show resolved Hide resolved
wire/msgmixpairreq.go Outdated Show resolved Hide resolved
wire/msgmixsecrets.go Show resolved Hide resolved
wire/msgmixsecrets.go Outdated Show resolved Hide resolved
wire/msgmixsecrets.go Outdated Show resolved Hide resolved
wire/msgmixslotreserve.go Outdated Show resolved Hide resolved
wire/msgmixslotreserve.go Outdated Show resolved Hide resolved
wire/msgmixslotreserve.go Show resolved Hide resolved
wire/msgmixslotreserve.go Outdated Show resolved Hide resolved
wire/msgmixslotreserve.go Outdated Show resolved Hide resolved
Copy link
Member

@davecgh davecgh left a comment

Choose a reason for hiding this comment

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

Just a couple of minor things left and this is ready to go in.

wire/msgmixkeyexchange.go Outdated Show resolved Hide resolved
wire/msgmixsecrets.go Outdated Show resolved Hide resolved
@jrick
Copy link
Member Author

jrick commented Feb 22, 2024

One note before this does go in, I am considering removing the expiry field from all messages except the pair request (and from the mixing message interface). The expiry checks on the other messages are generally useless because we must receive KEs even if they reference unknown PRs (but we will require them to reference at least one known PR), and there's no way to verify that the stated expiry is the minimum of all PRs when some are unknown. We will still expire sessions that reference an expired PR, so we don't need to rely on each message's individual expiry field.

@davecgh
Copy link
Member

davecgh commented Feb 22, 2024

No objections to removing the Expiry to me and it will result in a slightly more efficient messages. I'll hold off until you do that then.

Copy link
Member

@davecgh davecgh left a comment

Choose a reason for hiding this comment

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

Updates to remove expiry look good. This can be squashed and it looks ready to go in, so I'll get it approved and merged after that.

These messages implement the stages of a cspp mix.  Messages will be
broadcast to all full nodes and all peers participating in mixing
through inventory messages.
@davecgh davecgh merged commit b9d8d49 into decred:master Mar 19, 2024
2 checks passed
@davecgh davecgh added the wire protocol change Discussion and pull requests regarding items that require changes to the wire protocol. label May 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
wire protocol change Discussion and pull requests regarding items that require changes to the wire protocol.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants