Atomic proposal application (#3207)

src/node/gov.h

@@ -104,4 +104,31 @@ namespace ccf
     DECLARE_JSON_TYPE(Ballot);
     DECLARE_JSON_REQUIRED_FIELDS(Ballot, ballot);
   }
+}
+
+namespace fmt
+{
+  template <>
+  struct formatter<std::optional<ccf::jsgov::Failure>>
+  {
+    template <typename ParseContext>
+    constexpr auto parse(ParseContext& ctx)
+    {
+      return ctx.begin();
+    }
+
+    template <typename FormatContext>
+    auto format(const std::optional<ccf::jsgov::Failure>& f, FormatContext& ctx)
+    {
+      if (f.has_value())
+      {
+        return format_to(
+          ctx.out(), "{}\nTrace: {}", f->reason, f->trace.value_or("N/A"));
+      }
+      else
+      {
+        return format_to(ctx.out(), "N/A");
+      }
+    }
+  };
 }

src/node/rpc/member_frontend.h

@@ -985,19 +985,33 @@ namespace ccf
           proposal_id,
           ctx.rpc_ctx->get_request_body(),
           constitution.value());
-        pi->put(
-          proposal_id,
-          {caller_identity.member_id,
-           rv.state,
-           {},
-           {},
-           std::nullopt,
-           rv.failure});
 
-        ctx.rpc_ctx->set_response_status(HTTP_STATUS_OK);
-        ctx.rpc_ctx->set_response_header(
-          http::headers::CONTENT_TYPE, http::headervalues::contenttype::JSON);
-        ctx.rpc_ctx->set_response_body(nlohmann::json(rv).dump());
+        if (rv.state == ProposalState::FAILED)
+        {
+          // If the proposal failed to apply, we want to discard the tx and not
+          // apply its side-effects to the KV state.
+          ctx.rpc_ctx->set_error(
+            HTTP_STATUS_INTERNAL_SERVER_ERROR,
+            ccf::errors::InternalError,
+            fmt::format("{}", rv.failure));
+          return;
+        }
+        else
+        {
+          pi->put(
+            proposal_id,
+            {caller_identity.member_id,
+             rv.state,
+             {},
+             {},
+             std::nullopt,
+             rv.failure});
+          ctx.rpc_ctx->set_response_header(
+            http::headers::CONTENT_TYPE, http::headervalues::contenttype::JSON);
+          ctx.rpc_ctx->set_response_body(nlohmann::json(rv).dump());
+          ctx.rpc_ctx->set_response_status(HTTP_STATUS_OK);
+          return;
+        }
       };
 
       make_endpoint("/proposals", HTTP_POST, post_proposals_js, member_sig_only)
@@ -1197,17 +1211,16 @@ namespace ccf
         .set_auto_schema<void, jsgov::Proposal>()
         .install();
 
-      auto vote_js = [this](
-                       endpoints::EndpointContext& ctx,
-                       nlohmann::json&& params) {
+      auto vote_js = [this](ccf::endpoints::EndpointContext& ctx) {
         const auto& caller_identity =
           ctx.get_caller<ccf::MemberSignatureAuthnIdentity>();
         if (!check_member_active(ctx.tx, caller_identity.member_id))
         {
-          return make_error(
+          ctx.rpc_ctx->set_error(
             HTTP_STATUS_FORBIDDEN,
             ccf::errors::AuthorizationFailed,
             "Member is not active.");
+          return;
         }
 
         ProposalId proposal_id;
@@ -1215,17 +1228,21 @@ namespace ccf
         if (!get_proposal_id_from_path(
               ctx.rpc_ctx->get_request_path_params(), proposal_id, error))
         {
-          return make_error(
-            HTTP_STATUS_BAD_REQUEST, ccf::errors::InvalidResourceName, error);
+          ctx.rpc_ctx->set_error(
+            HTTP_STATUS_BAD_REQUEST,
+            ccf::errors::InvalidResourceName,
+            std::move(error));
+          return;
         }
 
         auto constitution = ctx.tx.ro(network.constitution)->get();
         if (!constitution.has_value())
         {
-          return make_error(
+          ctx.rpc_ctx->set_error(
             HTTP_STATUS_INTERNAL_SERVER_ERROR,
             ccf::errors::InternalError,
             "No constitution is set - proposals cannot be evaluated");
+          return;
         }
 
         auto pi =
@@ -1233,15 +1250,16 @@ namespace ccf
         auto pi_ = pi->get(proposal_id);
         if (!pi_)
         {
-          return make_error(
+          ctx.rpc_ctx->set_error(
             HTTP_STATUS_NOT_FOUND,
             ccf::errors::ProposalNotFound,
             fmt::format("Could not find proposal {}.", proposal_id));
+          return;
         }
 
         if (pi_.value().state != ProposalState::OPEN)
         {
-          return make_error(
+          ctx.rpc_ctx->set_error(
             HTTP_STATUS_BAD_REQUEST,
             ccf::errors::ProposalNotOpen,
             fmt::format(
@@ -1250,6 +1268,7 @@ namespace ccf
               proposal_id,
               pi_.value().state,
               ProposalState::OPEN));
+          return;
         }
 
         auto pm = ctx.tx.ro<ccf::jsgov::ProposalMap>(Tables::PROPOSALS);
@@ -1257,21 +1276,25 @@ namespace ccf
 
         if (!p)
         {
-          return make_error(
+          ctx.rpc_ctx->set_error(
             HTTP_STATUS_NOT_FOUND,
             ccf::errors::ProposalNotFound,
             fmt::format("Proposal {} does not exist.", proposal_id));
+          return;
         }
 
         if (pi_->ballots.find(caller_identity.member_id) != pi_->ballots.end())
         {
-          return make_error(
+          ctx.rpc_ctx->set_error(
             HTTP_STATUS_BAD_REQUEST,
             ccf::errors::VoteAlreadyExists,
             "Vote already submitted.");
+          return;
         }
         // Validate vote
 
+        auto params = nlohmann::json::parse(ctx.rpc_ctx->get_request_body());
+
         {
           js::Runtime rt;
           js::Context context(rt);
@@ -1289,18 +1312,32 @@ namespace ccf
 
         auto rv = resolve_proposal(
           ctx.tx, proposal_id, p.value(), constitution.value());
-        pi_.value().state = rv.state;
-        pi_.value().final_votes = rv.votes;
-        pi_.value().vote_failures = rv.vote_failures;
-        pi_.value().failure = rv.failure;
-        pi->put(proposal_id, pi_.value());
-        return make_success(rv);
+        if (rv.state == ProposalState::FAILED)
+        {
+          // If the proposal failed to apply, we want to discard the tx and not
+          // apply its side-effects to the KV state.
+          ctx.rpc_ctx->set_error(
+            HTTP_STATUS_INTERNAL_SERVER_ERROR,
+            ccf::errors::InternalError,
+            fmt::format("{}", rv.failure));
+          return;
+        }
+        else
+        {
+          pi_.value().state = rv.state;
+          pi_.value().final_votes = rv.votes;
+          pi_.value().vote_failures = rv.vote_failures;
+          pi_.value().failure = rv.failure;
+          pi->put(proposal_id, pi_.value());
+          ctx.rpc_ctx->set_response_header(
+            http::headers::CONTENT_TYPE, http::headervalues::contenttype::JSON);
+          ctx.rpc_ctx->set_response_body(nlohmann::json(rv).dump());
+          ctx.rpc_ctx->set_response_status(HTTP_STATUS_OK);
+          return;
+        }
       };
       make_endpoint(
-        "/proposals/{proposal_id}/ballots",
-        HTTP_POST,
-        json_adapter(vote_js),
-        member_sig_only)
+        "/proposals/{proposal_id}/ballots", HTTP_POST, vote_js, member_sig_only)
         .set_auto_schema<jsgov::Ballot, jsgov::ProposalInfoSummary>()
         .install();
 

tests/governance_js.py

@@ -8,6 +8,7 @@ import infra.e2e_args
 import suite.test_requirements as reqs
 import os
 from loguru import logger as LOG
+import pprint
 
 
 def action(name, **args):
@@ -498,7 +499,7 @@ def test_actions(network, args):
     return network
 
 
-@reqs.description("Test apply")
+@reqs.description("Test resolve and apply failures")
 def test_apply(network, args):
     node = network.find_random_node()
 
@@ -507,23 +508,48 @@ def test_apply(network, args):
             "/gov/proposals",
             proposal(action("always_throw_in_apply")),
         )
-        assert r.status_code == 200, r.body.text()
-        assert r.body.json()["state"] == "Failed", r.body.json()
+        assert r.status_code == 500, r.body.text()
+        assert r.body.json()["error"]["code"] == "InternalError", r.body.json()
         assert (
-            r.body.json()["failure"]["reason"]
+            r.body.json()["error"]["message"].split("\n")[0]
             == "Failed to apply(): Error: Error message"
         ), r.body.json()
 
+    with node.client(None, "member0") as c:
+        pprint.pprint(
+            proposal(action("always_accept_noop"), action("always_throw_in_apply"))
+        )
+        r = c.post(
+            "/gov/proposals",
+            proposal(action("always_accept_noop"), action("always_throw_in_apply")),
+        )
+        assert r.status_code == 200, r.body().text()
+        proposal_id = r.body.json()["proposal_id"]
+        r = c.post(f"/gov/proposals/{proposal_id}/ballots", ballot_yes)
+        assert r.status_code == 200, r.body().text()
+
+        with node.client(None, "member1") as c:
+            r = c.post(f"/gov/proposals/{proposal_id}/ballots", ballot_yes)
+            assert r.body.json()["error"]["code"] == "InternalError", r.body.json()
+            assert (
+                "Failed to apply():" in r.body.json()["error"]["message"]
+            ), r.body.json()
+            assert (
+                "Error: Error message" in r.body.json()["error"]["message"]
+            ), r.body.json()
+
     with node.client(None, "member0") as c:
         r = c.post(
             "/gov/proposals",
             proposal(action("always_throw_in_resolve")),
         )
-        assert r.status_code == 200, r.body.text()
-        assert r.body.json()["state"] == "Failed", r.body.json()
+        assert r.status_code == 500, r.body.text()
+        assert r.body.json()["error"]["code"] == "InternalError", r.body.json()
         assert (
-            r.body.json()["failure"]["reason"]
-            == "Failed to resolve(): Error: Resolve message"
+            "Failed to resolve():" in r.body.json()["error"]["message"]
+        ), r.body.json()
+        assert (
+            "Error: Resolve message" in r.body.json()["error"]["message"]
         ), r.body.json()
 
     return network

tests/membership.py

@@ -184,7 +184,9 @@ def recovery_shares_scenario(args):
             test_remove_member(network, args, recovery_member=True)
             assert False, "Removing a recovery member should not be possible"
         except infra.proposal.ProposalNotAccepted as e:
-            assert e.proposal.state == infra.proposal.ProposalState.FAILED
+            # This is an apply() time failure, so the proposal remains Open
+            # since the last vote is effectively discarded
+            assert e.proposal.state == infra.proposal.ProposalState.OPEN
 
         # However, removing a non-recovery member is allowed
         LOG.info("Removing a non-recovery member is still possible")
@@ -254,7 +256,9 @@ def recovery_shares_scenario(args):
                 False
             ), "Setting recovery threshold to more than number of active recovery members should not be possible"
         except infra.proposal.ProposalNotAccepted as e:
-            assert e.proposal.state == infra.proposal.ProposalState.FAILED
+            # This is an apply() time failure, so the proposal remains Open
+            # since the last vote is effectively discarded
+            assert e.proposal.state == infra.proposal.ProposalState.OPEN
 
         try:
             test_set_recovery_threshold(network, args, recovery_threshold=256)