Move context lock into issueTx #2524

StephenButtolph · 2023-12-20T20:38:35Z

Why this should be merged

We are moving the context lock acquisition into network.IssueTx. This PR separates this work to make moving the lock through boundaries clearly correct.

How this works

Ensures that vm.issueTx does not hold the context lock.
Ensures the wallet service maintains a consistent view of the UTXO set by holding the context lock throughout the issuance. This means we will no longer perform state checks against transactions issued over the wallet service... However I don't think this is an important change in behavior.

How this was tested

CI (the json string changed because we are no longer parsing the tx, so the tx provided from the node was initialized manually rather than by the codec parser)

StephenButtolph · 2023-12-20T20:47:37Z

vms/avm/service_test.go

@@ -622,7 +622,7 @@ func TestServiceGetTxJSON_CreateAssetTx(t *testing.T) {

 	// contains the address in the right format
 	require.Contains(jsonString, `"outputs":[{"addresses":["X-testing1lnk637g0edwnqc2tn8tel39652fswa3xk4r65e"],"groupID":1,"locktime":0,"threshold":1},{"addresses":["X-testing1lnk637g0edwnqc2tn8tel39652fswa3xk4r65e"],"groupID":2,"locktime":0,"threshold":1}]}`)
-	require.Contains(jsonString, `"initialStates":[{"fxIndex":0,"fxID":"spdxUxVJQbX85MGxMHbKw1sHxMnSqJ3QBzDyDYEP3h6TLuxqQ","outputs":[{"addresses":["X-testing1lnk637g0edwnqc2tn8tel39652fswa3xk4r65e"],"locktime":0,"threshold":1},{"addresses":["X-testing1lnk637g0edwnqc2tn8tel39652fswa3xk4r65e"],"locktime":0,"threshold":1}]},{"fxIndex":1,"fxID":"qd2U4HDWUvMrVUeTcCHp6xH3Qpnn1XbU5MDdnBoiifFqvgXwT","outputs":[{"addresses":["X-testing1lnk637g0edwnqc2tn8tel39652fswa3xk4r65e"],"groupID":1,"locktime":0,"threshold":1},{"addresses":["X-testing1lnk637g0edwnqc2tn8tel39652fswa3xk4r65e"],"groupID":2,"locktime":0,"threshold":1}]},{"fxIndex":2,"fxID":"rXJsCSEYXg2TehWxCEEGj6JU2PWKTkd6cBdNLjoe2SpsKD9cy","outputs":[{"addresses":["X-testing1lnk637g0edwnqc2tn8tel39652fswa3xk4r65e"],"locktime":0,"threshold":1},{"addresses":["X-testing1lnk637g0edwnqc2tn8tel39652fswa3xk4r65e"],"locktime":0,"threshold":1}]}]},"credentials":[],"id":"2MDgrsBHMRsEPa4D4NA1Bo1pjkVLUK173S3dd9BgT2nCJNiDuS"}`)
+	require.Contains(jsonString, `"initialStates":[{"fxIndex":0,"fxID":"spdxUxVJQbX85MGxMHbKw1sHxMnSqJ3QBzDyDYEP3h6TLuxqQ","outputs":[{"addresses":["X-testing1lnk637g0edwnqc2tn8tel39652fswa3xk4r65e"],"locktime":0,"threshold":1},{"addresses":["X-testing1lnk637g0edwnqc2tn8tel39652fswa3xk4r65e"],"locktime":0,"threshold":1}]},{"fxIndex":1,"fxID":"qd2U4HDWUvMrVUeTcCHp6xH3Qpnn1XbU5MDdnBoiifFqvgXwT","outputs":[{"addresses":["X-testing1lnk637g0edwnqc2tn8tel39652fswa3xk4r65e"],"groupID":1,"locktime":0,"threshold":1},{"addresses":["X-testing1lnk637g0edwnqc2tn8tel39652fswa3xk4r65e"],"groupID":2,"locktime":0,"threshold":1}]},{"fxIndex":2,"fxID":"rXJsCSEYXg2TehWxCEEGj6JU2PWKTkd6cBdNLjoe2SpsKD9cy","outputs":[{"addresses":["X-testing1lnk637g0edwnqc2tn8tel39652fswa3xk4r65e"],"locktime":0,"threshold":1},{"addresses":["X-testing1lnk637g0edwnqc2tn8tel39652fswa3xk4r65e"],"locktime":0,"threshold":1}]}]},"credentials":null,"id":"2MDgrsBHMRsEPa4D4NA1Bo1pjkVLUK173S3dd9BgT2nCJNiDuS"}`)


This is needed because we no longer parse the tx in issueTx

StephenButtolph · 2023-12-20T20:48:45Z

vms/avm/vm.go

-	if !vm.bootstrapped || vm.Builder == nil {
-		return ids.ID{}, errBootstrapping
-	}


This isn't needed anymore. We only allow API traffic after bootstrapping has finished, and we are guaranteed to have Linearize called during bootstrapping.

StephenButtolph · 2023-12-20T20:48:59Z

vms/avm/vm.go

-// If onDecide is specified, the function will be called when the transaction is
-// either accepted or rejected with the appropriate status. This function will
-// go out of scope when the transaction is removed from memory.


This comment was very wrong... probably stale from years ago

StephenButtolph · 2023-12-20T20:50:11Z

vms/avm/wallet_service.go

 	for {
 		txID, tx, ok := w.pendingTxs.Oldest()
 		if !ok {
 			return
 		}

-		txBytes := tx.Bytes()
-		_, err := w.vm.IssueTx(txBytes)
+		err := w.vm.mempool.Add(tx)


This is a behavior change. Previously we would validate the tx against the current state... now we just issue the tx into the mempool. Because this is only for transactions issued over the wallet API (which is deprecated) I think this minor change is fine.

StephenButtolph · 2023-12-20T20:50:28Z

vms/avm/wallet_service.go

 		if err == nil {
 			w.vm.ctx.Log.Info("issued tx to mempool over wallet API",
 				zap.Stringer("txID", txID),
 			)
+
+			w.vm.mempool.RequestBuildBlock()


We may want to just include this inside of Add in the mempool.

StephenButtolph · 2023-12-20T20:50:57Z

vms/avm/wallet_service.go

@@ -82,11 +76,16 @@ func (w *WalletService) issue(txBytes []byte) (ids.ID, error) {
 	}

 	if w.pendingTxs.Len() == 0 {
-		_, err := w.vm.IssueTx(txBytes)
-		if err != nil {
+		if err := w.vm.mempool.Add(tx); err != nil {


This is again a small behavior change - same rationale as above as to why I think it is fine.

StephenButtolph · 2023-12-20T20:52:14Z

vms/avm/wallet_service.go

@@ -31,27 +31,26 @@ type WalletService struct {
 }

 func (w *WalletService) decided(txID ids.ID) {


This is called during block.Accept - so we must support holding the context lock here.

joshua-kim · 2023-12-20T20:50:36Z

vms/avm/environment_test.go

+	vm.ctx.Lock.Unlock()
+	txID, err := vm.issueTx(tx)
+	vm.ctx.Lock.Lock()


Why can't we modify the tests calling this to just not hold the lock?

I'll make a PR on top of this PR and we can decide which we prefer

StephenButtolph · 2023-12-20T20:53:50Z

vms/avm/wallet_service.go

@@ -302,7 +306,7 @@ func (w *WalletService) SendMultiple(_ *http.Request, args *SendMultipleArgs, re
 		return err
 	}

-	txID, err := w.issue(tx.Bytes())
+	txID, err := w.issue(tx)


We need to ensure atomic access between update and issue - so we use the context lock for exclusion throughout this API.

vms/avm/vm.go

patrick-ogrady · 2023-12-20T21:58:24Z

vms/avm/wallet_service.go

 		if err == nil {
 			w.vm.ctx.Log.Info("issued tx to mempool over wallet API",
 				zap.Stringer("txID", txID),
 			)
+


afaict, it wouldn't be easy to change this to be called after all txs in a block have onAccept/onReject on them

…s#2524)

Move context lock into issueTx

95d0331

StephenButtolph self-assigned this Dec 20, 2023

StephenButtolph added this to the v1.10.18 milestone Dec 20, 2023

StephenButtolph added the vm This involves virtual machines label Dec 20, 2023

StephenButtolph commented Dec 20, 2023

View reviewed changes

joshua-kim reviewed Dec 20, 2023

View reviewed changes

StephenButtolph commented Dec 20, 2023

View reviewed changes

joshua-kim mentioned this pull request Dec 20, 2023

Move context lock into network.issueTx #2525

Merged

danlaine approved these changes Dec 20, 2023

View reviewed changes

patrick-ogrady reviewed Dec 20, 2023

View reviewed changes

vms/avm/vm.go Outdated Show resolved Hide resolved

ok

9bfa50d

patrick-ogrady reviewed Dec 20, 2023

View reviewed changes

merged

cedb056

dhrubabasu approved these changes Dec 20, 2023

View reviewed changes

StephenButtolph enabled auto-merge December 20, 2023 22:17

StephenButtolph added this pull request to the merge queue Dec 20, 2023

Merged via the queue into dev with commit fca1a16 Dec 20, 2023

StephenButtolph deleted the move-issuance-lock branch December 20, 2023 22:37

tedim52 pushed a commit to tedim52/avalanchego that referenced this pull request Oct 9, 2024

Standardize stakeAmount and weight usage in P-chain APIs (ava-lab…

cf7edbb

…s#2524)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move context lock into issueTx #2524

Move context lock into issueTx #2524

StephenButtolph commented Dec 20, 2023

StephenButtolph Dec 20, 2023

StephenButtolph Dec 20, 2023

StephenButtolph Dec 20, 2023

StephenButtolph Dec 20, 2023

StephenButtolph Dec 20, 2023

StephenButtolph Dec 20, 2023

StephenButtolph Dec 20, 2023

joshua-kim Dec 20, 2023

StephenButtolph Dec 20, 2023

StephenButtolph Dec 20, 2023

StephenButtolph Dec 20, 2023

patrick-ogrady Dec 20, 2023

		@@ -31,27 +31,26 @@ type WalletService struct {
		}

		func (w *WalletService) decided(txID ids.ID) {

Move context lock into issueTx #2524

Move context lock into issueTx #2524

Conversation

StephenButtolph commented Dec 20, 2023

Why this should be merged

How this works

How this was tested

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