add more governance testcases, governance bug fixes

hook/govern.c

@@ -1,6 +1,6 @@
 #include "hookapi.h"
 #define ASSERT(x)\
-    if ((x) < 0)\
+    if (!(x))\
         rollback(SBUF("Govern: Assertion failed."),__LINE__);
 
 #define SEAT_COUNT 20
@@ -152,7 +152,7 @@ int64_t hook(uint32_t r)
         TRACEVAR(imc);
         
         // set member count
-        ASSERT(state_set(SVAR(imc), "MC", 2));
+        ASSERT(0 < state_set(SVAR(imc), "MC", 2));
 
         member_count = imc;
         TRACEVAR(member_count);
@@ -176,10 +176,10 @@ int64_t hook(uint32_t r)
                 NOPE("Governance: Initial Reward Delay must be > 0.");
         
             // set reward rate
-            ASSERT(state_set(SVAR(irr), "RR", 2));
+            ASSERT(0 < state_set(SVAR(irr), "RR", 2));
 
             // set reward delay
-            ASSERT(state_set(SVAR(ird), "RD", 2));
+            ASSERT(0 < state_set(SVAR(ird), "RD", 2));
         }
 
         for (uint8_t i = 0; GUARD(SEAT_COUNT), i < member_count; ++i)
@@ -327,7 +327,7 @@ int64_t hook(uint32_t r)
 
         state(&votes, 1, topic_data, 32);
         votes++;
-        ASSERT(state_set(&votes, 1, topic_data, 32));
+        ASSERT(0 < state_set(&votes, 1, topic_data, 32));
 
         // restore the saved bytes
         *((uint64_t*)topic_data) = saved_data;
@@ -335,8 +335,12 @@ int64_t hook(uint32_t r)
    
 
     // set this flag if the topic data is all zeros
-    uint8_t zero[32];
+    int topic_data_zero = 
-    int topic_data_zero = BUFFER_EQUAL_32(topic_data, zero); 
+        *((uint64_t*)topic_data + 0) == 0 &&
+        *((uint64_t*)topic_data + 8) == 0 &&
+        *((uint64_t*)topic_data + 16) == 0 &&
+        *((uint64_t*)topic_data + 24) == 0;
+
 
     if (DEBUG)
     {
@@ -353,21 +357,27 @@ int64_t hook(uint32_t r)
     int64_t q80 = member_count * 0.8;
     int64_t q51 = member_count * 0.51;
 
-    if (l == 2)
+    if (q80 < 2)
+        q80 = 2;
+    
+    if (q51 < 2)
+        q51 = 2;
+
+    if (l == 1)
     {
        if (votes <
             (t == 'S'
                 ? q80                      // L1s have 80% threshold for membership/seat voting
                 : member_count))            // L1s have 100% threshold for all other voting
-        DONE("Governance: Vote for L2 topic recorded. Not yet enough votes to action.");
+        DONE("Governance: L1 vote record. Not yet enough votes to action.");
     }
-    else    // l == 1
+    else    // l == 2
     {
         lost_majority = previous_votes >= q51 && votes < q51;
         if (lost_majority)
-            trace(SBUF("Governance: Majority lost, undoing L1 vote."),0,0,0);
+            trace(SBUF("Governance: L2 vote recorded. Majority lost, undoing L1 vote."),0,0,0);
         else if (votes < q51)
-            DONE("Governance: Vote for L1 topic recorded. Not yet enough votes to action.");
+            DONE("Governance: L2 vote recorded. Not yet enough votes to action L1 vote..");
     }
 
     
@@ -461,7 +471,9 @@ int64_t hook(uint32_t r)
         case 'R':
         {
             // reward topics
-            ASSERT(state_set(topic_data + padding, topic_size, SBUF(topic)));
+            int64_t result = state_set(topic_data + padding, topic_size, SBUF(topic));
+            TRACEVAR(result);
+            ASSERT(0 < result);
             if (n == 'R')
                 DONE("Governance: Reward rate change actioned!");
             
@@ -537,7 +549,7 @@ int64_t hook(uint32_t r)
             if (previous_present && !topic_data_zero)
             {
                 // we will not change member count, we're adding a member and removing a member
-                
+                trace(SBUF("previous_present && !topic_data_zero"),0,0,0);
                 // pass
             }
             else
@@ -547,10 +559,13 @@ int64_t hook(uint32_t r)
                     member_count--;
                 else
                     member_count++;
-                
+   
-                ASSERT(member_count > 0);   // just bail out if the last member is trying to self remove
+                TRACEVAR(member_count);
 
-                ASSERT(state_set(&member_count, 1, SBUF(zero)) == 1);
+                ASSERT(member_count > 1);   // just bail out if the second last member is being removed
+
+                uint8_t mc = member_count;
+                ASSERT(state_set(&mc, 1, "MC", 2) == 1);
             }
 
             // we need to garbage collect all their votes

src/ripple/protocol/impl/STTx.cpp

@@ -34,6 +34,7 @@
 #include <ripple/protocol/TxFlags.h>
 #include <ripple/protocol/UintTypes.h>
 #include <ripple/protocol/jss.h>
+#include <ripple/app/hook/applyHook.h>
 #include <boost/format.hpp>
 #include <array>
 #include <memory>
@@ -425,6 +426,46 @@ STTx::checkMultiSign(
 static bool
 isMemoOkay(STObject const& st, std::string& reason)
 {
+
+    auto const maxKey = hook::maxHookParameterKeySize();
+    auto const maxVal = hook::maxHookParameterValueSize();
+
+    // piggyback otxn hookparameter checking here
+    if (st.isFieldPresent(sfHookParameters))
+    {
+        auto const& params = st.getFieldArray(sfHookParameters);
+        if (params.size() > 16)
+        {
+            reason = "hookParameter count must be less than 17.";
+            return false;
+        }
+
+        for (auto const& param : params)
+        {
+            auto paramObj = dynamic_cast<STObject const*>(&param);
+
+            if (!paramObj || (paramObj->getFName() != sfHookParameter))
+            {
+                reason = "A HookParameters array may contain only HookParameter objects.";
+                return false;
+            }
+
+            if (!paramObj->isFieldPresent(sfHookParameterName) ||
+                    paramObj->getFieldVL(sfHookParameterName).size() > maxKey)
+            {
+                reason = "HookParameterName cannot exceed 32 bytes.";
+                return false;
+            }
+
+            if (!paramObj->isFieldPresent(sfHookParameterValue) ||
+                    paramObj->getFieldVL(sfHookParameterValue).size() > maxVal)
+            {
+                reason = "HookParameterValue cannot exceed 128 bytes.";
+                return false;
+            }
+        }
+    }
+
     if (!st.isFieldPresent(sfMemos))
         return true;
 

src/test/app/XahauGenesis_test.cpp

@@ -427,49 +427,198 @@ struct XahauGenesis_test : public beast::unit_test::suite
         };
 
 
-        auto const kl =
+        auto doL1Vote = [&](Account const& acc, char topic1, char topic2, 
-            keylet::hookState(env.master.id(), makeStateKey('V', 'R', 'R', 1, alice.id()),
+                            std::vector<uint8_t> const& vote_data,
-                uint256("0000000000000000000000000000000000000000000000000000000000000000"));
+                            std::vector<uint8_t> const& old_data,
-        auto entry = env.le(kl);
+                            bool actioned = true, bool const shouldFail = false)
-        BEAST_EXPECT(!entry);
-
-        // alice votes for a different reward rate
-
-        std::vector<uint8_t> vote_data{0x00U,0x81U,0xC6U,0xA4U,0x7EU,0x8DU,0x43U,0x54U};
-        env(vote(alice, 'R', 'R', vote_data), fee(XRP(1)));
-        env.close();
-
-        //VRR...000...alice.id
-
-        //hookState(AccountID const& id, uint256 const& key, uint256 const& ns) noexcept;
-        entry = env.le(kl);
-
-        BEAST_REQUIRE(!!entry);
-
-        auto data = entry->getFieldVL(sfHookStateData);
-        BEAST_EXPECT(data.size() == 8);
-
-        BEAST_EXPECT(data == vote_data);
-        
-        auto doL1Vote = [&](Account const& acc, char topic1, char topic2, std::vector<uint8_t> const& data) -> void
         {
-            env(vote(acc, 'R', 'R', vote_data), fee(XRP(1)));
-            env.close();
-            auto entry = env.le(keylet::hookState(env.master.id(), makeStateKey('V', 'R', 'R', 1, acc.id()),
-                uint256("0000000000000000000000000000000000000000000000000000000000000000")));
-            BEAST_REQUIRE(!!entry);
-            auto lgr_data = entry->getFieldVL(sfHookStateData);
-            BEAST_EXPECT(lgr_data.size() == vote_data.size());
-            BEAST_EXPECT(lgr_data == vote_data);
-        };
-        // bob votes the same way
-        doL1Vote(bob, 'R', 'R', vote_data);
-    
-        // ... etc until 100%
-        doL1Vote(carol, 'R', 'R', vote_data);
-        doL1Vote(david, 'R', 'R', vote_data);
-        doL1Vote(edward, 'R', 'R', vote_data);
 
+            if (shouldFail)
+                actioned = false;
+
+            uint8_t const key[32] = {0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0, topic1, topic2};
+            // check actioning prior to vote
+            {
+                auto entry = env.le(keylet::hookState(env.master.id(), uint256::fromVoid(key), beast::zero)); 
+                BEAST_EXPECT((old_data.empty() && !entry) || 
+                    (entry && entry->getFieldVL(sfHookStateData) == old_data));
+            }    
+
+            // perform and check vote
+            {
+                env(vote(acc, topic1, topic2, vote_data), fee(XRP(1)),
+                    shouldFail ? ter(tecHOOK_REJECTED) : ter(tesSUCCESS));
+                env.close();
+                auto entry =
+                    env.le(keylet::hookState(env.master.id(), makeStateKey('V', topic1, topic2, 1, acc.id()),
+                        beast::zero));
+                if (!shouldFail)
+                {
+                    BEAST_REQUIRE(!!entry);
+                    auto lgr_data = entry->getFieldVL(sfHookStateData);
+                    BEAST_EXPECT(lgr_data.size() == vote_data.size());
+                    BEAST_EXPECT(lgr_data == vote_data);
+                }
+            }
+
+            // check actioning
+            {
+                // if the vote count isn't high enough it will be hte old value if it's high enough it will be the
+                // new value
+                auto entry = env.le(keylet::hookState(env.master.id(), uint256::fromVoid(key), beast::zero));
+            
+                bool isZero = true;
+                for (auto&  x: vote_data)
+                if (x != 0)
+                {
+                    isZero = false;
+                    break;
+                }
+                    
+
+                if (!actioned && old_data.empty())
+                {
+                    BEAST_EXPECT(!entry);
+                    return;
+                }
+
+                if (actioned)
+                {
+                    if (isZero)
+                        BEAST_EXPECT(!entry);
+                    else
+                        BEAST_EXPECT(!!entry && entry->getFieldVL(sfHookStateData) == vote_data);
+                }
+                else
+                {
+                    std::cout << "old data: " << strHex(entry->getFieldVL(sfHookStateData)) << "\n";
+                    BEAST_EXPECT(entry->getFieldVL(sfHookStateData) == old_data);
+                }
+            }
+        };
+
+            
+        // 100% vote for a different reward rate
+        {
+            // this will be the new reward rate
+            std::vector<uint8_t> vote_data {0x00U,0x81U,0xC6U,0xA4U,0x7EU,0x8DU,0x43U,0x54U};
+            
+            // this is the default reward rate
+            std::vector<uint8_t> const original_data {0x00U,0xE4U,0x61U,0xEEU,0x78U,0x90U,0x83U,0x54U};
+            
+            doL1Vote(alice, 'R', 'R', vote_data, original_data, false);                
+            doL1Vote(bob, 'R', 'R', vote_data, original_data, false);
+            doL1Vote(carol, 'R', 'R', vote_data, original_data, false);
+            doL1Vote(david, 'R', 'R', vote_data, original_data, false);
+            doL1Vote(edward, 'R', 'R', vote_data, original_data, true);
+
+            // reverting a vote should not undo the action
+            doL1Vote(carol, 'R', 'R', original_data, vote_data, false);
+
+            // submitting a null vote should delete the vote, and should not undo the action
+            std::vector<uint8_t> const null_data {0,0,0,0,0,0,0,0};
+            doL1Vote(david, 'R', 'R', null_data, vote_data, false);
+        }
+
+        
+        uint8_t const member_count_key[32] = {0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0, 0,0,0,0,0,0,0,0,0,0,0,0,0,0,'M','C'};
+        std::vector<uint8_t> const null_acc_id {0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0};
+
+        // four of the 5 vote to remove alice
+        {
+            std::vector<uint8_t> const null_acc_id {0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0};
+            std::vector<uint8_t> id;
+            id.reserve(20);
+            memcpy(id.data(), alice.id().data(), 20);
+            doL1Vote(bob, 'S', 0, null_acc_id, id, false);
+            doL1Vote(carol, 'S', 0, null_acc_id, id, false);
+            doL1Vote(david, 'S', 0, null_acc_id, id, false);
+            doL1Vote(edward, 'S', 0, null_acc_id, id, true);
+        }
+
+
+        // check the membercount is now 4
+        {
+            auto entry = env.le(keylet::hookState(env.master.id(),
+                uint256::fromVoid(member_count_key), beast::zero));
+            std::vector<uint8_t> const expected_data {0x04U};
+            BEAST_REQUIRE(!!entry);
+            BEAST_EXPECT(entry->getFieldVL(sfHookStateData) == expected_data);
+        }
+
+        // continue to remove members (david)
+        {
+            std::vector<uint8_t> id;
+            id.reserve(20);
+            memcpy(id.data(), david.id().data(), 20);
+            doL1Vote(bob, 'S', 3, null_acc_id, id, false);
+            doL1Vote(carol, 'S', 3, null_acc_id, id, false);
+            doL1Vote(edward, 'S', 3, null_acc_id, id, true);
+        }
+
+        // check the membercount is now 3
+        {
+            auto entry = env.le(keylet::hookState(env.master.id(),
+                uint256::fromVoid(member_count_key), beast::zero));
+            std::vector<uint8_t> const expected_data {0x03U};
+            BEAST_REQUIRE(!!entry);
+            BEAST_EXPECT(entry->getFieldVL(sfHookStateData) == expected_data);
+        }
+
+        // continue to remove members (carol)
+        {
+            std::vector<uint8_t> id;
+            id.reserve(20);
+            memcpy(id.data(), carol.id().data(), 20);
+            doL1Vote(bob, 'S', 2, null_acc_id, id, false);
+            doL1Vote(edward, 'S', 2, null_acc_id, id, true);
+        }
+        
+        // check the membercount is now 2
+        {
+            auto entry = env.le(keylet::hookState(env.master.id(),
+                uint256::fromVoid(member_count_key), beast::zero));
+            std::vector<uint8_t> const expected_data {0x02U};
+            BEAST_REQUIRE(!!entry);
+            BEAST_EXPECT(entry->getFieldVL(sfHookStateData) == expected_data);
+        }
+
+        // member count can't fall below 2
+        // try to vote out edward using 2/2 votes
+        {
+            std::vector<uint8_t> id;
+            id.reserve(20);
+            memcpy(id.data(), edward.id().data(), 20);
+            doL1Vote(bob, 'S', 4, null_acc_id, id, true);
+            doL1Vote(edward, 'S', 4, null_acc_id, id, false, true);
+        }
+
+        // check the membercount is now 2
+        {
+            auto entry = env.le(keylet::hookState(env.master.id(),
+                uint256::fromVoid(member_count_key), beast::zero));
+            std::vector<uint8_t> const expected_data {0x02U};
+            BEAST_REQUIRE(!!entry);
+            BEAST_EXPECT(entry->getFieldVL(sfHookStateData) == expected_data);
+        }
+
+        // try to remove bob using 1/2 votes
+        {
+            std::vector<uint8_t> id;
+            id.reserve(20);
+            memcpy(id.data(), bob.id().data(), 20);
+            doL1Vote(bob, 'S', 1, null_acc_id, id, false, false);
+        }
+
+        // that shoul fail
+        // check the membercount is now 2
+        {
+            auto entry = env.le(keylet::hookState(env.master.id(),
+                uint256::fromVoid(member_count_key), beast::zero));
+            std::vector<uint8_t> const expected_data {0x02U};
+            BEAST_REQUIRE(!!entry);
+            BEAST_EXPECT(entry->getFieldVL(sfHookStateData) == expected_data);
+        }
             
     }