From aac36aeacd4eda5fdb491cbb9b84e36b08d41154 Mon Sep 17 00:00:00 2001 From: Eshel Date: Mon, 25 Sep 2023 01:32:54 +0300 Subject: [PATCH] tested what happens if submsg recipient suceeds but caller fails via a faulty sdk msg. once when this msg is added on the reply endpoint, and once on the regular execute --- .../test-compute-contract/src/contract.rs | 149 +++++++++++++----- .../test-compute-contract/src/msg.rs | 2 + go-cosmwasm/types/systemerror.go | 4 +- x/compute/internal/keeper/msg_dispatcher.go | 2 +- .../keeper/secret_contracts_exec_test.go | 2 +- .../keeper/secret_contracts_migrate_test.go | 4 +- .../keeper/secret_contracts_submsgs_test.go | 122 ++++++++++++-- 7 files changed, 228 insertions(+), 57 deletions(-) diff --git a/cosmwasm/contracts/v1/compute-tests/test-compute-contract/src/contract.rs b/cosmwasm/contracts/v1/compute-tests/test-compute-contract/src/contract.rs index 801ced867..5cb10525c 100644 --- a/cosmwasm/contracts/v1/compute-tests/test-compute-contract/src/contract.rs +++ b/cosmwasm/contracts/v1/compute-tests/test-compute-contract/src/contract.rs @@ -645,18 +645,50 @@ pub fn execute(deps: DepsMut, env: Env, info: MessageInfo, msg: ExecuteMsg) -> S } ExecuteMsg::IncrementAndSendFailingSubmessage { reply_on } => { increment_simple(deps)?; - let mut response = send_failing_submsg(env, reply_on)?; - response.data = Some((count as u32).to_be_bytes().into()); + let response = send_failing_submsg(env, reply_on)?; Ok(response) }, ExecuteMsg::IncrementAndSendSubmessageWithBankFail { reply_on } => { increment_simple(deps)?; - let mut response = send_failing_submsg_with_bank_fail(env, reply_on)?; - response.data = Some((count as u32).to_be_bytes().into()); + let response = send_failing_submsg_with_bank_fail(env, reply_on)?; Ok(response) }, + ExecuteMsg::SendSucceedingSubmessageThenFailingMessageOnReply {} => { + increment_simple(deps)?; + + let mut response = Response::default(); + add_succeeding_submsg(env, &mut response, ReplyOn::Always, 9201); + + // failing message is created on reply + Ok(response) + }, + ExecuteMsg::SendSucceedingSubmessageAndFailingMessage {} => { + increment_simple(deps)?; + + let mut response = Response::default(); + + // add_succeeding_submsg(env, &mut response, ReplyOn::Always, 9201); + + // response = response.add_message( + // CosmosMsg::Bank(BankMsg::Send { + // to_address: "non-existent".to_string(), + // amount: vec![Coin::new(100, "non-existent")], + // }) + // ); + + response = response.add_message( + CosmosMsg::Wasm(WasmMsg::Execute { + contract_addr: env.contract.address.clone().into_string(), + code_hash: env.contract.code_hash.clone(), + msg: Binary::from(r#"{"quick_error":{}}"#.as_bytes().to_vec()), + funds: vec![], + }) + ); + + Ok(response) + } ExecuteMsg::InitV10 { counter, code_id, @@ -1767,6 +1799,34 @@ pub fn send_multiple_sub_messages_with_reply_with_error( Ok(resp) } +pub fn add_succeeding_submsg( + env: Env, + resp: &mut Response, + reply_on: ReplyOn, + id: u64, +) { + let message = ExecuteMsg::Increment { addition: 3 }; + + let message = Binary::from( + serde_json_wasm::to_string(&message) + .unwrap() + .as_bytes() + .to_vec() + ); + + resp.messages.push(SubMsg { + id: id, + msg: CosmosMsg::Wasm(WasmMsg::Execute { + contract_addr: env.contract.address.clone().into_string(), + code_hash: env.contract.code_hash.clone(), + msg: message, + funds: vec![], + }), + gas_limit: Some(10000000_u64), + reply_on, + }); +} + pub fn send_failing_submsg_with_bank_fail( env: Env, reply_on: ReplyOn, @@ -1785,7 +1845,7 @@ pub fn send_failing_submsg_with_bank_fail( let mut resp = Response::default(); resp.messages.push(SubMsg { - id: 9200, + id: 9202, msg: CosmosMsg::Wasm(WasmMsg::Execute { contract_addr: env.contract.address.clone().into_string(), code_hash: env.contract.code_hash.clone(), @@ -2336,9 +2396,28 @@ pub fn reply(deps: DepsMut, env: Env, reply: Reply) -> StdResult { (9200, SubMsgResult::Err(_)) => Ok(Response::default().set_data( (count_read(deps.storage).load()? as u32).to_be_bytes() )), - (11337, SubMsgResult::Ok(SubMsgResponse { data, .. })) => { + (9201, _) => { + // check that the submessage worked + if count_read(deps.storage).load()? != 14 { + return Ok(Response::default()); // test expects error, so this will fail the test + } + + increment_simple(deps)?; + let response = Response::default().add_message( + CosmosMsg::Bank(BankMsg::Send { + to_address: "non-existent".to_string(), + amount: vec![Coin::new(100, "non-existent")], + }) + ); + Ok(response) + }, + (9202, _) => { + increment_simple(deps)?; + Ok(Response::default()) + }, + (11337, SubMsgResult::Ok(SubMsgResponse { data, .. })) => { let ( contract_addr, new_code_id, callback_code_hash, @@ -2360,10 +2439,6 @@ pub fn reply(deps: DepsMut, env: Env, reply: Reply) -> StdResult { }, }; - - - - Ok( Response::new().add_message(CosmosMsg::Wasm(WasmMsg::Migrate { contract_addr, @@ -2375,46 +2450,36 @@ pub fn reply(deps: DepsMut, env: Env, reply: Reply) -> StdResult { } (11338, SubMsgResult::Ok(SubMsgResponse { data, .. })) => { let contract_addr= match from_binary(&data.unwrap()) { - Ok(ExecuteMsg::SendMsgClearAdmin { - contract_addr, - .. - }) => contract_addr, - - Ok(_) => { - return Err(StdError::generic_err("cannot parse into SendMsgClearAdmin")); - - } - Err(err) => { - return Err(StdError::generic_err(format!("cannot parse into SendMsgClearAdmin: {:?}",err))); + Ok(ExecuteMsg::SendMsgClearAdmin { + contract_addr, + .. + }) => contract_addr, - }, - }; + Ok(_) => { + return Err(StdError::generic_err("cannot parse into SendMsgClearAdmin")); + } + Err(err) => { + return Err(StdError::generic_err(format!("cannot parse into SendMsgClearAdmin: {:?}",err))); + }, + }; Ok(Response::new().add_message(CosmosMsg::Wasm(WasmMsg::ClearAdmin { contract_addr }))) } (11339, SubMsgResult::Ok(SubMsgResponse { data, .. })) => { - let ( contract_addr, - new_admin)= match from_binary(&data.unwrap()) { - Ok(ExecuteMsg::SendMsgUpdateAdmin { - contract_addr, - new_admin, - .. - }) => ( contract_addr, - new_admin), + let (contract_addr, new_admin) = match from_binary(&data.unwrap()) { + Ok(ExecuteMsg::SendMsgUpdateAdmin { + contract_addr, + new_admin, + .. + }) => (contract_addr, new_admin), Ok(_) => { - return Err(StdError::generic_err("cannot parse into SendMsgUpdateAdmin")); - + return Err(StdError::generic_err("cannot parse into SendMsgUpdateAdmin")); } - Err(err) => { - return Err(StdError::generic_err(format!("cannot parse into SendMsgUpdateAdmin: {:?}",err))); - - }, - }; - - - - + Err(err) => { + return Err(StdError::generic_err(format!("cannot parse into SendMsgUpdateAdmin: {:?}",err))); + }, + }; Ok(Response::new().add_message( CosmosMsg::Wasm(WasmMsg::UpdateAdmin { contract_addr, diff --git a/cosmwasm/contracts/v1/compute-tests/test-compute-contract/src/msg.rs b/cosmwasm/contracts/v1/compute-tests/test-compute-contract/src/msg.rs index 4696d1e60..e9190a151 100644 --- a/cosmwasm/contracts/v1/compute-tests/test-compute-contract/src/msg.rs +++ b/cosmwasm/contracts/v1/compute-tests/test-compute-contract/src/msg.rs @@ -181,6 +181,8 @@ pub enum ExecuteMsg { IncrementAndSendSubmessageWithBankFail { reply_on: ReplyOn, }, + SendSucceedingSubmessageThenFailingMessageOnReply {}, + SendSucceedingSubmessageAndFailingMessage {}, InitV10 { code_id: u64, code_hash: String, diff --git a/go-cosmwasm/types/systemerror.go b/go-cosmwasm/types/systemerror.go index 064c9ae18..65aaf886e 100644 --- a/go-cosmwasm/types/systemerror.go +++ b/go-cosmwasm/types/systemerror.go @@ -90,8 +90,8 @@ func (e ExceededRecursionLimit) Error() string { return "unknown system error" } -// ToSystemError will try to convert the given error to an SystemError. -// This is important to returning any Go error back to Rust. +// ToSystemError will try to convert the given error to a SystemError. +// This is important for returning any Go error back to Rust. // // If it is already StdError, return self. // If it is an error, which could be a sub-field of StdError, embed it. diff --git a/x/compute/internal/keeper/msg_dispatcher.go b/x/compute/internal/keeper/msg_dispatcher.go index 9a925d6db..32b13338a 100644 --- a/x/compute/internal/keeper/msg_dispatcher.go +++ b/x/compute/internal/keeper/msg_dispatcher.go @@ -312,7 +312,7 @@ func (d MessageDispatcher) DispatchSubmessages(ctx sdk.Context, contractAddr sdk // In a case when the reply is encrypted but the sdk failed (Most likely, funds issue) // we return a error if isReplyEncrypted(msg) && isSdkError { - return nil, fmt.Errorf("an sdk error occoured while sending a sub-message: %s", redactedErr.Error()) + return nil, fmt.Errorf("an sdk error occurred while sending a sub-message: %s", redactedErr.Error()) } if isReplyEncrypted(msg) { diff --git a/x/compute/internal/keeper/secret_contracts_exec_test.go b/x/compute/internal/keeper/secret_contracts_exec_test.go index ff05d9f22..a5f4df2b1 100644 --- a/x/compute/internal/keeper/secret_contracts_exec_test.go +++ b/x/compute/internal/keeper/secret_contracts_exec_test.go @@ -1594,7 +1594,7 @@ func TestV1SendsFundsWithErrorWithReply(t *testing.T) { _, _, _, _, _, err := execHelper(t, keeper, ctx, contractAddress, walletA, privKeyA, `{"send_funds_with_error_with_reply":{}}`, false, true, math.MaxUint64, 0) require.NotEmpty(t, err) - require.Contains(t, fmt.Sprintf("%+v", err), "an sdk error occoured while sending a sub-message") + require.Contains(t, fmt.Sprintf("%+v", err), "an sdk error occurred while sending a sub-message") } func TestCallbackSanity(t *testing.T) { diff --git a/x/compute/internal/keeper/secret_contracts_migrate_test.go b/x/compute/internal/keeper/secret_contracts_migrate_test.go index 5638d923d..9319610b7 100644 --- a/x/compute/internal/keeper/secret_contracts_migrate_test.go +++ b/x/compute/internal/keeper/secret_contracts_migrate_test.go @@ -1579,7 +1579,7 @@ func TestV1SendsFundsWithErrorWithReplyAfterMigrate(t *testing.T) { _, _, _, _, _, err := execHelper(t, keeper, ctx, contractAddress, walletA, privKeyA, `{"send_funds_with_error_with_reply":{}}`, false, testContract.IsCosmWasmV1After, math.MaxUint64, 0) require.NotEmpty(t, err) - require.Contains(t, fmt.Sprintf("%+v", err), "an sdk error occoured while sending a sub-message") + require.Contains(t, fmt.Sprintf("%+v", err), "an sdk error occurred while sending a sub-message") }) } } @@ -3899,7 +3899,7 @@ func TestV1SendsFundsWithErrorWithReplyDuringMigrate(t *testing.T) { _, err := migrateHelper(t, keeper, ctx, newCodeId, contractAddress, walletA, privKeyA, `{"send_funds_with_error_with_reply":{}}`, false, testContract.IsCosmWasmV1After, math.MaxUint64) require.NotEmpty(t, err) - require.Contains(t, fmt.Sprintf("%+v", err), "an sdk error occoured while sending a sub-message") + require.Contains(t, fmt.Sprintf("%+v", err), "an sdk error occurred while sending a sub-message") }) } } diff --git a/x/compute/internal/keeper/secret_contracts_submsgs_test.go b/x/compute/internal/keeper/secret_contracts_submsgs_test.go index a208c8d10..a7a3f3e2f 100644 --- a/x/compute/internal/keeper/secret_contracts_submsgs_test.go +++ b/x/compute/internal/keeper/secret_contracts_submsgs_test.go @@ -3,7 +3,6 @@ package keeper import ( "encoding/binary" "encoding/json" - "fmt" "math" "testing" @@ -43,6 +42,19 @@ func TestV1MultipleSubmessagesNoReply(t *testing.T) { require.Equal(t, uint32(16), resp.Get.Count) } +/* +Test case: + 0. Initial counter stored in state: 10 + 1. Contract increments 1 (=11) + 2. Contract sends itself submessage, with reply_on=always + 3. Submessage execution sets counter to 123456 + 4. Submessage fails with error + 5. Contract's reply() returns Ok + +Expected Outcome: + - Counter is still on 11, revert only submessage changes + - No Error (tx as a whole succeeds - counter will stay at 11) +*/ func TestV1StatePersistsAfterSubmessageFails(t *testing.T) { ctx, keeper, codeID, _, walletA, privKeyA, _, _ := setupTest(t, TestContractPaths[v1Contract], sdk.NewCoins()) @@ -61,7 +73,12 @@ func TestV1StatePersistsAfterSubmessageFails(t *testing.T) { require.Equal(t, uint32(11), resp.Get.Count) } -func TestV1StatePersistsAfterSubmessageFailsNoReply(t *testing.T) { +/* +Test case: Same as previous one but there's no reply. +Expected Outcome: Since the caller contract is not executed again, the final result of this message is determined by +the submessage recipient. It errors, so the whole transaction will revert. +*/ +func TestV1StateRevertsAfterSubmessageFailsAndNoReply(t *testing.T) { ctx, keeper, codeID, _, walletA, privKeyA, _, _ := setupTest(t, TestContractPaths[v1Contract], sdk.NewCoins()) _, _, contractAddress, _, _ := initHelper(t, keeper, ctx, codeID, walletA, nil, privKeyA, `{"counter":{"counter":10, "expires":100}}`, true, true, defaultGasForTests) @@ -75,31 +92,118 @@ func TestV1StatePersistsAfterSubmessageFailsNoReply(t *testing.T) { var resp v1QueryResponse e := json.Unmarshal([]byte(queryRes), &resp) require.NoError(t, e) + // The state here should have been reverted by the APP but in go-tests we create our own keeper so it is not reverted + // in this case. Only the submessage changes revert, since we manage them in a sub-context in our keeper. require.Equal(t, uint32(11), resp.Get.Count) } -func TestV1StatePersistsAfterSubmessageThatGeneratesBankMsgFails(t *testing.T) { +/* +Test case: + 0. Initial counter stored in state: 10 + 1. Contract increments 1 (=11) + 2. Contract sends itself submessage with reply=always + 3. Submessage execution increments 1 (=12) + 4. Submessage adds a failing bank message to the messages list + 5. Submessage returns Ok, since the bank message did not execute yet + 6. Bank message fails on go side + Note: If the reply was called, it would have incremented the counter again (to 12) + +Observed Outcome: + - Contract does not handle reply, even though it is reply_on=always, because sdk messages revert the whole tx. + This is because regular messages are not postponed until all Submsgs are processed. Instead, they are processed + in the same ordering as SubMsgs. This differs from cosmwasm's documentation at https://github.com/CosmWasm/cosmwasm/blob/main/SEMANTICS.md#order-and-rollback + The difference in outcomes is small when sdk message fails, because either way, the whole tx is reverted, but may + be present in cases of contract-queries-to-chain if the sdk message succeeds. + + - Counter is still on 11. SubMsg changes reverted, reply not reached. (first increment remains as it is reverted in sdk) + - Error is not empty, whole tx will revert because of sdk message failing. +*/ +func TestV1StateRevertsAfterSubmessageThatGeneratesBankMsgFails(t *testing.T) { ctx, keeper, codeID, _, walletA, privKeyA, _, _ := setupTest(t, TestContractPaths[v1Contract], sdk.NewCoins()) - fmt.Println("ESHELDEBUG before init") //todo remove _, _, contractAddress, _, _ := initHelper(t, keeper, ctx, codeID, walletA, nil, privKeyA, `{"counter":{"counter":10, "expires":100}}`, true, true, defaultGasForTests) - fmt.Println("ESHELDEBUG before exec") //todo remove - _, _, _, _, _, err := execHelper(t, keeper, ctx, contractAddress, walletA, privKeyA, `{"increment_and_send_submessage_with_bank_fail":{"reply_on":"never"}}`, false, true, math.MaxUint64, 0) + _, _, _, _, _, err := execHelper(t, keeper, ctx, contractAddress, walletA, privKeyA, `{"increment_and_send_submessage_with_bank_fail":{"reply_on":"always"}}`, false, true, math.MaxUint64, 0) - fmt.Println("ESHELDEBUG before error check") //todo remove require.NotEmpty(t, err) - fmt.Println("ESHELDEBUG after error check") //todo remove queryRes, qErr := queryHelper(t, keeper, ctx, contractAddress, `{"get":{}}`, true, true, math.MaxUint64) - fmt.Println("ESHELDEBUG after query") //todo remove require.Empty(t, qErr) var resp v1QueryResponse e := json.Unmarshal([]byte(queryRes), &resp) require.NoError(t, e) + + // The state here should have been reverted by the APP but in go-tests we create our own keeper so it is not reverted + // in this case. Only the submessage changes revert, since we manage them in a sub-context in our keeper. require.Equal(t, uint32(11), resp.Get.Count) } +/* +Test case: + 0. Initial counter stored in state: 10 + 1. Contract increments 1 (=11) + 2. Contract sends: + 2a. Submessage to itself: + 2a-1. Submessage execution increments counter by 3 (=14) + 2a-2. Submessage succeeds + 2b. Failing Bank message. + +Expected Result: + - Error is not empty - will revert all changes + - Counter is on 11 (main message revert happens in sdk, not reflected here) +*/ +func TestV1SubmessageStateRevertsIfCallerFails(t *testing.T) { + ctx, keeper, codeID, _, walletA, privKeyA, _, _ := setupTest(t, TestContractPaths[v1Contract], sdk.NewCoins()) + + _, _, contractAddress, _, _ := initHelper(t, keeper, ctx, codeID, walletA, nil, privKeyA, `{"counter":{"counter":10, "expires":100}}`, true, true, defaultGasForTests) + _, _, _, _, _, err := execHelper(t, keeper, ctx, contractAddress, walletA, privKeyA, `{"send_succeeding_submessage_and_failing_message":{}}`, true, true, math.MaxUint64, 0) + + require.NotEmpty(t, err) + + queryRes, qErr := queryHelper(t, keeper, ctx, contractAddress, `{"get":{}}`, true, true, math.MaxUint64) + require.Empty(t, qErr) + + var resp v1QueryResponse + e := json.Unmarshal([]byte(queryRes), &resp) + require.NoError(t, e) + + // The state here should have been reverted by the APP but in go-tests we create our own keeper so it is not reverted + // in this case. Only the submessage changes revert, since we manage them in a sub-context in our keeper. + require.Equal(t, uint32(11), resp.Get.Count) +} + +/* +Test case: + 0. Initial counter stored in state: 10 + 1. Contract increments 1 (=11) + 2. Contract sends itself submessage + 3. Submessage execution increments counter by 3 (=14) + 4. Submessage succeeds + 5. Contract receives reply + 6. Contract increments 1 again (=15) + 7. In reply, Contract issues a failing bank message to the go code + +Expected Result: + - Error is not empty - will revert all changes + - Counter is on 15 - whole tx revert happens on sdk +*/ +func TestV1SubmessageStateRevertsIfCallerFailsOnReply(t *testing.T) { + ctx, keeper, codeID, _, walletA, privKeyA, _, _ := setupTest(t, TestContractPaths[v1Contract], sdk.NewCoins()) + + _, _, contractAddress, _, _ := initHelper(t, keeper, ctx, codeID, walletA, nil, privKeyA, `{"counter":{"counter":10, "expires":100}}`, true, true, defaultGasForTests) + _, _, _, _, _, err := execHelper(t, keeper, ctx, contractAddress, walletA, privKeyA, `{"send_succeeding_submessage_then_failing_message_on_reply":{}}`, false, true, math.MaxUint64, 0) + + require.NotEmpty(t, err) + + queryRes, qErr := queryHelper(t, keeper, ctx, contractAddress, `{"get":{}}`, true, true, math.MaxUint64) + require.Empty(t, qErr) + + var resp v1QueryResponse + e := json.Unmarshal([]byte(queryRes), &resp) + require.NoError(t, e) + require.Equal(t, uint32(15), resp.Get.Count) +} + func TestSendEncryptedAttributesFromInitWithoutSubmessageWithoutReply(t *testing.T) { for _, testContract := range testContracts { t.Run(testContract.CosmWasmVersion, func(t *testing.T) {