Fetch tspends in SPV mode #2194

bgptr · 2022-12-01T09:53:31Z

This fetches tspends via the p2p network in SPV mode using a getinitstate message and adds them to the tspends cache.

This fetches tspends via the p2p network in SPV mode using a `getinitstate` message and adds them to the tspend cache.

p2p/peering.go

wallet/wallet.go

jrick · 2022-12-01T14:10:35Z

Also, tspend transactions that are published after the initstate is handled need to be added to the wallet as well. There needs to be tspend handling added to the spv.Syncer.handleTxInvs method.

matheusd

Tested current version on simnet and it worked as intended (tspend arrived to the wallet in spv mode).

matheusd · 2022-12-01T13:21:03Z

p2p/peering.go

@@ -1678,3 +1695,16 @@ func (rp *RemotePeer) SendHeadersSent() bool {
 	rp.requestedHeadersMu.Unlock()
 	return sent
 }
+
+// GetInitState attempts to get initial state by sending a NewMsgGetInitState message.


[...] by sending a GetInitState message.

NewMsgGetInitState is the name of the func, the message itself is GetInitState.

matheusd · 2022-12-01T13:23:16Z

p2p/peering.go

+func (rp *RemotePeer) GetInitState(ctx context.Context) error {
+	const opf = "remotepeer(%v).GetInitState"
+	msg := wire.NewMsgGetInitState()
+	msg.AddTypes(wire.InitStateHeadBlocks, wire.InitStateHeadBlockVotes, wire.InitStateTSpends)


You can probably just request wire.InitStateTSpends and not the other types here, because they are not used for anything in spv mode.

matheusd · 2022-12-01T13:48:09Z

p2p/peering.go

@@ -771,6 +773,8 @@ func (rp *RemotePeer) readMessages(ctx context.Context) error {
 				rp.receivedGetMiningState(ctx)
 			case *wire.MsgGetInitState:
 				rp.receivedGetInitState(ctx)
+			case *wire.MsgInitState:
+				rp.lp.receivedInitState <- newInMsg(rp, msg)


You need to select across ctx.Done() as well, otherwise this goroutine may leak.

I suggest following the same pattern as the other handlers, introducing a rp.receivedInitState() func and handling everything there.

Another thing you might wanna consider is adding checks to ensure the MsgInitState was in fact received as a response to a previous GetInitState (and not just arbitrarily sent by the remote peer multiple times).

(I see jrick already commented something similar while I was writing this)

matheusd · 2022-12-01T13:54:38Z

spv/sync.go

+				for i, tx := range prevTxs {
+					if tx != nil {
+						tspendTxs = append(tspendTxs, tx)
+						unseenTSpends = append(unseenTSpends, prevUnseen[i])


It doesn't look like you're using the unseenTSpends slice after this point, only tspendTxs, so it seems unnecessary to also recreate this slice.

matheusd · 2022-12-01T14:31:47Z

spv/sync.go

+				tspendTxs, unseenTSpends = tspendTxs[:0], unseenTSpends[:0]
+				for i, tx := range prevTxs {
+					if tx != nil {
+						tspendTxs = append(tspendTxs, tx)


You need to double check the received transaction is actually a tspend by calling stake.IsTSpend, and ideally also check the signature is valid for tspend keys for the current network and that it has not expired.

- instroduce MaskInitState and mask MsgInitState typed messages with it - add comment to IsTSpendCached public function - reword comment (NewMsgGetInitState -> GetInitState) - just request wire.InitStateTSpends type - remove unused unseenTSpends - double check the received transaction is actually a tspend and that it has not expired. - handle incoming tspends in handleTxInvs

…ous GetInitState (and not just arbitrarily sent by the remote peer multiple times)

…e is valid.

matheusd · 2022-12-02T14:15:16Z

wallet/wallet.go

@@ -232,6 +232,14 @@ func (w *Wallet) VotingEnabled() bool {
 	return enabled
 }

+// IsTSpendCached returns wheater the given hash is already cached


wheater => whether

End sentence with a period.

matheusd · 2022-12-02T14:26:29Z

p2p/peering.go

+		rp.Disconnect(err)
+		rp.requestedInitStateMu.Unlock()


Swap these two lines, so that you do the Disconnect() after releasing the mutex.

There's no obvious race doing the current way, but in general you should try to do as little as possible inside a critical section (that is, while having a locked mutex), for safety and future proofing.

In fact, you can rewrite the use of the requestedInitStateMu mutex in this function to keep it really short:

rp.requestedInitStateMu.Lock() c := rp.requestedInitState rp.requestedInitState = nil rp.requestedInitStateMu.Unlock() if c == nil { // error out }

Writing unconditionally to the field (rp.requestedInitState = nil) is fine because if it's already nil, it will remain so and if it's non-nil, c will have the channel for the later code to act upon. This makes the time spent in the critical section the smallest possible and makes it easier to deduce there will be no deadlocks possible (because there are no function calls that could themselves trigger a deadlock).

matheusd · 2022-12-02T14:37:50Z

p2p/peering.go

+			go func() {
+				<-stalled.C
+				rp.deleteRequestedInitState()
+			}()


It's not clear to me why you're doing the deleteRequestedInitState inside a goroutine.

If the context is done, you can just call the deleteRequestedInitState() directly (because no reply is expected anymore). Doing after waiting for stalled.C to trigger is just leaking this goroutine for potentially 30 seconds.

matheusd · 2022-12-02T14:41:33Z

p2p/peering.go

+			rp.Disconnect(err)
+			return nil, err
+		case <-rp.errc:
+			stalled.Stop()


This potentially leaks timer resources forever (check the doc for the Stop() function).

You need to call this like the doc explains:

if !stalled.Stop() { <-stalled.C }

matheusd · 2022-12-02T14:42:06Z

p2p/peering.go

+		case out <- &msgAck{msg, nil}:
+			out = nil
+		case msg := <-c:
+			stalled.Stop()


Same as above

matheusd · 2022-12-02T14:46:28Z

spv/sync.go

@@ -1410,6 +1527,11 @@ func (s *Syncer) startupSync(ctx context.Context, rp *p2p.RemotePeer) error {
 		}
 	}

+	err = s.GetInitState(ctx, rp)


This should only be called if rp.Pver() >= wire.InitStateVersion. The current peer code has a minimum supported version that is lower than the one that introduced the GetInitState message, so we could be connecting to a peer that does not support it.

jrick · 2022-12-02T16:28:51Z

p2p/peering.go

+func (rp *RemotePeer) GetInitState(ctx context.Context) (*wire.MsgInitState, error) {
+	const opf = "remotepeer(%v).GetInitState"
+	msg := wire.NewMsgGetInitState()
+	msg.AddTypes(wire.InitStateTSpends)


let's either move these type constants into the method parameters, so it can be used to query anything (we might want to fetch other mempool txs using this, too), or take the *wire.MsgGetInitState as a parameter directly.

jrick · 2022-12-02T16:30:26Z

spv/sync.go

+	sigHash, err := txscript.CalcSignatureHash(nil,
+		txscript.SigHashAll, msgTx, 0, nil)
+	if err != nil {
+		return fmt.Errorf("CalcSignatureHash: %w", err)


use decred.org/dcrwallet/v3/errors

matheusd · 2022-12-05T11:26:24Z

spv/sync.go

+		if s.checkTSpend(ctx, tx) {
+			s.wallet.AddTSpend(*tx)
+		}


Was doing another functional test for this case and I realized this section is only called if the tspend has an output relevant to the wallet.

This needs to be moved into a loop of its own, before filtering for relevant transactions, otherwise the wallet will not know about the tspend to set a voting policy for.

check tspends before filtering for relevant txs

Fetch tspends in SPV mode

4c2988d

This fetches tspends via the p2p network in SPV mode using a `getinitstate` message and adds them to the tspend cache.

bgptr mentioned this pull request Dec 1, 2022

[governance] Introduce TSpend voting decred/decrediton#3787

Merged

improve checking if a tspend already cached

0c15b55

jrick requested changes Dec 1, 2022

View reviewed changes

p2p/peering.go Outdated Show resolved Hide resolved

wallet/wallet.go Show resolved Hide resolved

matheusd reviewed Dec 1, 2022

View reviewed changes

bgptr added 5 commits December 1, 2022 18:38

ensure the MsgInitState was in fact received as a response to a previ…

4a602f6

…ous GetInitState (and not just arbitrarily sent by the remote peer multiple times)

nuke ReceiveInitState function

2ad5fcc

Verify that the TSpend uses a well-known Pi key and that the signatur…

9f69b10

…e is valid.

cleanup

c45d4dc

bgptr requested a review from jrick December 2, 2022 13:59

matheusd reviewed Dec 2, 2022

View reviewed changes

review comments (@matheusd)

d58161c

jrick requested changes Dec 2, 2022

View reviewed changes

review comments (@jrick)

d215a94

matheusd reviewed Dec 5, 2022

View reviewed changes

review comment (@matheusd)

10119a5

check tspends before filtering for relevant txs

jrick approved these changes Dec 6, 2022

View reviewed changes

jrick merged commit eab54c3 into decred:master Dec 6, 2022

bgptr deleted the tspend-spv branch December 6, 2022 20:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fetch tspends in SPV mode #2194

Fetch tspends in SPV mode #2194

bgptr commented Dec 1, 2022

jrick commented Dec 1, 2022

matheusd left a comment

matheusd Dec 1, 2022

matheusd Dec 1, 2022

matheusd Dec 1, 2022

matheusd Dec 1, 2022

matheusd Dec 1, 2022

matheusd Dec 1, 2022

matheusd Dec 2, 2022

matheusd Dec 2, 2022

matheusd Dec 2, 2022

matheusd Dec 2, 2022

matheusd Dec 2, 2022

matheusd Dec 2, 2022

jrick Dec 2, 2022

jrick Dec 2, 2022

matheusd Dec 5, 2022

Fetch tspends in SPV mode #2194

Fetch tspends in SPV mode #2194

Conversation

bgptr commented Dec 1, 2022

jrick commented Dec 1, 2022

matheusd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment