Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: NodeUpdate needs only admin key to sign #16495

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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`
* for the account assigned as the current "node account".
Copy link
Member

@jsync-swirlds jsync-swirlds Nov 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Node account key is only required if that field is changing.
The admin_key is the signature that is always required.

Suggested change
* for the account assigned as the current "node account".
* 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 @@ -261,7 +261,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
Loading