Skip to content

Commit

Permalink
feat: (cherry-pick) NodeUpdate needs only admin key to sign (#16502)
Browse files Browse the repository at this point in the history
Signed-off-by: Iris Simon <[email protected]>
  • Loading branch information
iwsimon authored Nov 8, 2024
1 parent 883567b commit a21a5be
Show file tree
Hide file tree
Showing 8 changed files with 28 additions and 48 deletions.
34 changes: 12 additions & 22 deletions hapi/hedera-protobufs/services/address_book_service.proto
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ import "transaction.proto";
* but each node operator may update their own operational attributes without
* additional approval, reducing overhead for routine operations.
*
* All operations are `privileged operations` and require governing council
* Most operations are `privileged operations` and require governing council
* approval.
*
* ### For a node creation transaction.
Expand Down Expand Up @@ -83,29 +83,19 @@ import "transaction.proto";
* be active in the network following this upgrade.
*
* ### For a node update transaction.
* - The node operator or Hedera council representative SHALL create an
* `updateNode` transaction.
* - If the node operator creates the transaction
* - The node operator MUST sign this transaction with the active `key`
* for the account assigned as the current "node account".
* - If the transaction changes the value of the "node account" the
* node operator MUST _also_ sign this transaction with the active `key`
* for the account to be assigned as the new "node account".
* - The node operator MUST deliver the signed transaction to the Hedera
* council representative.
* - The Hedera council representative SHALL arrange for council members to
* review and sign the transaction.
* - Once sufficient council members have signed the transaction, the
* Hedera council representative SHALL submit the transaction to the
* network.
* - The node operator SHALL create an `updateNode` transaction.
* - The node operator MUST sign this transaction with the active `key`
* assigned as the `admin_key`.
* - The node operator SHALL submit the transaction to the
* network. Hedera council approval SHALL NOT be sought for this
* transaction
* - Upon receipt of a valid and signed node update transaction the network
* software SHALL
* - Validate the threshold signature for the Hedera governing council
* - Validate the signature of the active `key` for the account to be
* assigned as the "node account".
* - If the transaction modifies the value of the "node account",
* - Validate the signature of the _new_ `key` for the account to be
* assigned as the new "node account".
* - Validate the signature of the active `key` for the account
* assigned as the _current_ "node account".
* - Validate the signature of the active `key` for the account to be
* assigned as the _new_ "node account".
* - Modify the node information held in network state with the changes
* requested in the update transaction. The node changes SHALL NOT be
* applied to network configuration, and SHALL NOT affect network
Expand Down Expand Up @@ -147,7 +137,7 @@ service AddressBookService {
* This transaction, once complete, SHALL modify the identified consensus
* node state as requested.
* <p>
* Hedera governing council authorization is REQUIRED for this transaction.
* This transaction is authorized by the node operator
*/
rpc updateNode (proto.Transaction) returns (proto.TransactionResponse);
}
7 changes: 3 additions & 4 deletions hapi/hedera-protobufs/services/node_update.proto
Original file line number Diff line number Diff line change
Expand Up @@ -28,10 +28,9 @@ import "basic_types.proto";
/**
* Transaction body to modify address book node attributes.
*
* This transaction body SHALL be considered a "privileged transaction".
*
* - This transaction MUST be signed by the governing council and
* - the active `admin_key` for the node.
* - This transaction SHALL enable the node operator, as identified by the
* `admin_key`, to modify operational attributes of the node.
* - This transaction MUST be signed by the active `admin_key` for the node.
* - If this transaction sets a new value for the `admin_key`, then both the
* current `admin_key`, and the new `admin_key` MUST sign this transaction.
* - This transaction SHALL NOT change any field that is not set (is null) in
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ public NodeUpdateHandler(@NonNull final AddressBookValidator addressBookValidato
@Override
public void pureChecks(@NonNull final TransactionBody txn) throws PreCheckException {
requireNonNull(txn);
final var op = txn.nodeUpdateOrThrow();
final var op = txn.nodeUpdate();
validateFalsePreCheck(op.nodeId() < 0, INVALID_NODE_ID);
if (op.hasGossipCaCertificate()) {
validateFalsePreCheck(op.gossipCaCertificate().equals(Bytes.EMPTY), INVALID_GOSSIP_CA_CERTIFICATE);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,7 @@ public SystemPrivilege hasPrivileges(
txBody.fileDeleteOrThrow().fileIDOrThrow().fileNum());
case CRYPTO_DELETE -> checkEntityDelete(
txBody.cryptoDeleteOrThrow().deleteAccountIDOrThrow().accountNumOrThrow());
case NODE_CREATE, NODE_UPDATE, NODE_DELETE -> checkNodeChange(payerId);
case NODE_CREATE, NODE_DELETE -> checkNodeChange(payerId);
default -> SystemPrivilege.UNNECESSARY;
};
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,6 @@
import com.hederahashgraph.api.proto.java.FreezeTransactionBody;
import com.hederahashgraph.api.proto.java.NodeCreateTransactionBody;
import com.hederahashgraph.api.proto.java.NodeDeleteTransactionBody;
import com.hederahashgraph.api.proto.java.NodeUpdateTransactionBody;
import com.hederahashgraph.api.proto.java.SignedTransaction;
import com.hederahashgraph.api.proto.java.SystemDeleteTransactionBody;
import com.hederahashgraph.api.proto.java.SystemUndeleteTransactionBody;
Expand Down Expand Up @@ -369,14 +368,6 @@ void addressBookAdminCanCreate() throws InvalidProtocolBufferException {
assertEquals(SystemOpAuthorization.AUTHORIZED, subject.authForTestCase(accessor(txn)));
}

@Test
void addressBookAdminCanUpdate() throws InvalidProtocolBufferException {
// given:
var txn = addressBookAdminTxn().setNodeUpdate(NodeUpdateTransactionBody.getDefaultInstance());
// expect:
assertEquals(SystemOpAuthorization.AUTHORIZED, subject.authForTestCase(accessor(txn)));
}

@Test
void addressBookAdminCanDelete() throws InvalidProtocolBufferException {
// given:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -260,7 +260,7 @@ public record ApiPermissionConfig(
@ConfigProperty(defaultValue = "0-*") PermissionedAccountsRange tokenCancelAirdrop,
@ConfigProperty(defaultValue = "0-*") PermissionedAccountsRange tokenClaimAirdrop,
@ConfigProperty(defaultValue = "2-55") PermissionedAccountsRange createNode,
@ConfigProperty(defaultValue = "2-55") PermissionedAccountsRange updateNode,
@ConfigProperty(defaultValue = "0-*") PermissionedAccountsRange updateNode,
@ConfigProperty(defaultValue = "2-55") PermissionedAccountsRange deleteNode,
@ConfigProperty(defaultValue = "0-0") PermissionedAccountsRange tssMessage,
@ConfigProperty(defaultValue = "0-0") PermissionedAccountsRange tssVote) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -375,14 +375,16 @@ final Stream<DynamicTest> signedByCouncilNotAdminKeyFail() throws CertificateEnc
}

@HapiTest
final Stream<DynamicTest> signedByCouncilAndAdminKeySuccess() throws CertificateEncodingException {
final Stream<DynamicTest> signedByAdminKeySuccess() throws CertificateEncodingException {
return hapiTest(
newKeyNamed("adminKey"),
cryptoCreate("payer").balance(10_000_000_000L),
nodeCreate("testNode")
.adminKey("adminKey")
.gossipCaCertificate(gossipCertificates.getFirst().getEncoded()),
nodeUpdate("testNode")
.signedBy(ADDRESS_BOOK_CONTROL, "adminKey")
.payingWith("payer")
.signedBy("payer", "adminKey")
.description("updated description")
.via("successUpdate"),
getTxnRecord("successUpdate").logged());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,6 @@
import static com.hedera.services.bdd.spec.utilops.EmbeddedVerbs.viewNode;
import static com.hedera.services.bdd.spec.utilops.UtilVerbs.newKeyNamed;
import static com.hedera.services.bdd.spec.utilops.UtilVerbs.validateChargedUsdWithin;
import static com.hedera.services.bdd.suites.HapiSuite.DEFAULT_PAYER;
import static com.hedera.services.bdd.suites.HapiSuite.ONE_HBAR;
import static com.hedera.services.bdd.suites.hip869.NodeCreateTest.generateX509Certificates;
import static com.hederahashgraph.api.proto.java.ResponseCodeEnum.INVALID_NODE_ACCOUNT_ID;
Expand Down Expand Up @@ -101,17 +100,16 @@ final Stream<DynamicTest> validateFees() throws CertificateEncodingException {
// Submit to a different node so ingest check is skipped
nodeUpdate("node100")
.setNode("0.0.5")
.signedBy(DEFAULT_PAYER)
.payingWith("payer")
.accountId("0.0.1000")
.fee(ONE_HBAR)
.hasKnownStatus(INVALID_SIGNATURE)
.via("failedUpdate"),
getTxnRecord("failedUpdate").logged(),
// The fee is not charged here because the payer is privileged
validateChargedUsdWithin("failedUpdate", 0.0, 3.0),
// The fee is charged here because the payer is not privileged
validateChargedUsdWithin("failedUpdate", 0.001, 3.0),
nodeUpdate("node100")
.adminKey("testKey")
.signedBy(DEFAULT_PAYER, "testKey")
.accountId("0.0.1000")
.fee(ONE_HBAR)
.via("updateNode"),
Expand All @@ -122,12 +120,12 @@ final Stream<DynamicTest> validateFees() throws CertificateEncodingException {
// Submit with several signatures and the price should increase
nodeUpdate("node100")
.setNode("0.0.5")
.signedBy(DEFAULT_PAYER, "payer", "payer", "randomAccount", "testKey")
.payingWith("payer")
.signedBy("payer", "payer", "randomAccount", "testKey")
.accountId("0.0.1000")
.fee(ONE_HBAR)
.via("failedUpdateMultipleSigs"),
// The fee is not charged here because the payer is privileged
validateChargedUsdWithin("failedUpdateMultipleSigs", 0.0, 3.0));
validateChargedUsdWithin("failedUpdateMultipleSigs", 0.0011276316, 3.0));
}

@EmbeddedHapiTest(NEEDS_STATE_ACCESS)
Expand Down

0 comments on commit a21a5be

Please sign in to comment.