feat: remove traces of account number from the account object

[zivkovicmilos]

Description

This PR removes the Account Number from the account object.

⚠️ BREAKING PR, PLEASE UPDATE ALL SIGNING CLIENTS. ⚠️

The account number is a sequential number generated from the internal chain state, to initialize accounts.
It serves no security benefits, given that the account object already has a Sequence for replay protection, and accounts are never deleted in TM2.

Having it introduces a large overhead on maintaining it, and not being able to return "empty" accounts. It also introduces friction when generating transaction signatures, as the client would need to go to the chain to fetch the active account number.

Starts the process of closing #661.

Related:

• Reminder: need to set VerifyGenesisSignatures to true on mainnet,  #1104
• change default genesis package creator, and add way to configure it #2573

Contributors' checklist...

• Added new tests, or not needed, or not feasible
• Provided an example (e.g. screenshot) to aid review or the PR is self-explanatory
• Updated the official documentation or not needed
• No breaking changes were made, or a BREAKING CHANGE: xxx message was included in the description
• Added references to related issues and PRs
• Provided any useful hints for running manual tests

[codecov]

Codecov Report

Attention: Patch coverage is 80.55556% with 7 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
tm2/pkg/std/tx.go	0.00%	6 Missing ⚠️
tm2/pkg/std/account.go	0.00%	1 Missing ⚠️

📢 Thoughts on this report? Let us know!

[moul]

There is a minor security benefit that may seem negligible. For example, similar to chainid, a faulty copy-paste of the gnokey command will fail if you attempt to run a transaction on the wrong network using the same key.

Having different chainid values should be enough to protect against cryptographic replay attacks. However, it also offers additional protection against operating on the incorrect network.

I need a bit more time to consider the impact.

Edit: I remember considering a similar approach. My idea was not to remove it but to make it optional, allowing you to choose between explicitness and implicitness. I wanted to extend this option to both the account number and the sequence number. However, the sequence number has a greater impact on security.

[moul]

should be kept.

[zivkovicmilos]

This replace was originally removed, not sure when it was added back in 🙁

The gnofaucet cannot have a replace, because gnolang/faucet (imported by gnofaucet) imports gnolang/gno, and it's going to cause a build error since the API changed in this version of gnolang/gno -- we want gnofaucet to use an old version of gnolang/gno

[thehowl]

thinking gnolang/faucet should be moved into the monorepo or gnofaucet out of it; in any case fine.

[moul]

Move it in the monorepo.

[zivkovicmilos]

There is a minor security benefit that may seem negligible. For example, similar to chainid, a faulty copy-paste of the gnokey command will fail if you attempt to run a transaction on the wrong network using the same key.

Having different chainid values should be enough to protect against cryptographic replay attacks. However, it also offers additional protection against operating on the incorrect network.

I need a bit more time to consider the impact.

Edit: I remember considering a similar approach. My idea was not to remove it but to make it optional, allowing you to choose between explicitness and implicitness. I wanted to extend this option to both the account number and the sequence number. However, the sequence number has a greater impact on security.

@moul

The only reasoning I can think of for keeping the Account Number is that at some point accounts would've been "deleted", and having a global account counter would prevent any funny business in the future. I can't see any limitation that sequence (changed on a per-tx basis on the account), and chain-id don't already provide as a combination (this is the case on Ethereum, this is the only replay protection).

The benefits of dropping it are we can minimize the amount of chain queries we do for regular transaction signing (ex. in Adena, or any signing client), and that we can now return values for uninitialized accounts (zero balance, zero sequence). It makes the signing logic cleaner and simpler

[moul]

I'm considering whether it makes sense to replace AccountNumber with the pubkey here. However, since we're signing with the corresponding privkey and appending the pubkey to the tx, it may be unnecessary.

cc @aeddi @kristovatlas

[aeddi]

I still lack some context and a deep knowledge of the codebase, but after some live discussions, reading the PR on Cosmos, and considering the arguments presented in the other comments, I feel that it's safe to remove this value. In fact, if it is truly unnecessary, it is important to eliminate it for the sake of clarity, especially when it comes to data related to security.

About adding a pub key field, there is no point in doing it IMO.

[moul]

Added several reviewers.

Blocking progress until Jae has a chance to review it. However, we shouldn't wait for Jae to continue the review.

[tbruyelle]

@zivkovicmilos As mentioned in the issue #661, why not return an error if the account doesn't exist ? This is what the auth module of the SDK does.

[thehowl]

overall lgtm and agree to remove it

[zivkovicmilos]

@zivkovicmilos As mentioned in the issue #661, why not return an error if the account doesn't exist ? This is what the auth module of the SDK does.

@tbruyelle
It's not just about the uninitialized account response, the bigger issue is we drag around the account number like we did the zero timestamp for no apparent reason or security benefit 🙁

I am actually totally fine with the uninitialized accounts erroring out like they do now, we handle this in all of our clients

[tbruyelle]

@tbruyelle It's not just about the uninitialized account response, the bigger issue is we drag around the account number like we did the zero timestamp for no apparent reason or security benefit 🙁

I am actually totally fine with the uninitialized accounts erroring out like they do now, we handle this in all of our clients

I see OK.

It is likely that the presence of the account number in gno is a simple port from the SDK codebase, and interestingly I was able to find the origin of the account number : cosmos/cosmos-sdk#1077

As noted in the PR, the account number was added as a security measure, actually a replay protection, but in the context of account pruning (account removed from state when balance is 0). However, I don't know much about state pruning so I can't say if this is still relevant. Maybe someone else can jump in and explain if this is still relevant.

[zivkovicmilos]

@tbruyelle

Following up after doing some digging and talking with people:

Accounts are never pruned from the SDK / Cosmos state, only blocks.

So we should be safe to remove Account Numbers 🫡

[Gno2D2]

I'm a bot that assists the Gno Core team in maintaining this repository. My role is to ensure that contributors understand and follow our guidelines, helping to streamline the development process.

The following requirements must be fulfilled before a pull request can be merged.
Some requirement checks are automated and can be verified by the CI, while others need manual verification by a staff member.

These requirements are defined in this configuration file.

Automated Checks

🔴 Maintainers must be able to edit this pull request
🔴 The pull request head branch must be up-to-date with its base
🔴 Changes to 'docs' folder must be reviewed/authored by at least one devrel and one tech-staff

Manual Checks

No manual checks match this pull request.

Debug

Automated Checks

Maintainers must be able to edit this pull request

If

🟢 Condition met
└── 🟢 On every pull request

Then

🔴 Requirement not satisfied
└── 🔴 Maintainer can modify this pull request

The pull request head branch must be up-to-date with its base

If

🟢 Condition met
└── 🟢 On every pull request

Then

🔴 Requirement not satisfied
└── 🔴 Head branch (dev/zivkovicmilos/account-number) is up to date with base (master): behind by 8 / ahead by 7

Changes to 'docs' folder must be reviewed/authored by at least one devrel and one tech-staff

If

🟢 Condition met
└── 🟢 A changed file matches this pattern: ^docs/ (filename: docs/gno-tooling/cli/gnokey/full-security-tx.md)

Then

🔴 Requirement not satisfied
└── 🔴 Or
    ├── 🔴 And
    │   ├── 🔴 Pull request author is a member of the team: devrels
    │   └── 🟢 At least 1 user(s) of the team tech-staff approved pull request
    └── 🔴 And
        ├── 🟢 Pull request author is a member of the team: tech-staff
        └── 🔴 At least 1 user(s) of the team devrels approved pull request

[moul]

Note about potentially related topics:

• Per-account GNOT locking + per-account blacklisting/unlocking with TOS signing Global GNOT lock #2980
• Per-account private key burning (indicating that the private key can no longer be used, e.g., when an account initializes with a key and transitions to a DAO) -> no issue yet, but i'll write one later
• Account session (managing multiple sets of public/private keys under a parent account) Account Sessions System (Cookie-Like) #1499

For these topics, using an account number may optimize storage and computing in some cases while allowing the --account-number flag to be optional or even removed entirely for users.

I am still considering the impact of this change. There is currently no strict blocker, and I prefer to make it optional or remove it altogether.

[zivkovicmilos]

@moul

Thinking about #1499

I think having an account number would actually be problematic in the sense of account abstraction, since an account object is something that currently needs to be initialized in the blockchain state, across nodes.

There is currently no strict blocker, and I prefer to make it optional or remove it altogether.

Making it optional will most likely introduce more overhead than actually keeping the account number outright, because we'd need to juggle 2 signing implementations at all times

[kristovatlas]

I have one more line of inquiry on this topic I'm researching, and then I'll chime in here.

[zivkovicmilos]

After discussing with @jaekwon, we've decided to not go in the direction of removing account numbers from Gno, but instead focus on having actually working account pruning from the client, which the account information makes easier to do.

I will do research on the storage impact for pruning empty accounts, and post my findings in a separate issue / PR 🙏

[kristovatlas]

Summary of my current understanding, for the sake of posterity:

In the Cosmos SDK (and later Gno via copy/paste), the code documents account numbers as providing replay protection for pruning nodes.

There are a few scenarios in which replay attacks can take place:

• Across networks, e.g. test4 and test5 of gno, but this is prevented by including the chain-id in the signed transaction message
• In the same network, within the same chain
• In the same network, during a chain split

Within the same network, the sequence number prevents replays of the same transaction. Pruning validators should not prune the current sequence number of any given account, and therefore pruning shouldn't be an issue. Account numbers shouldn't help here, either, because there is just one, consistent account number for each address.

During chain splits, things get slightly more complicated; two transactions in partitioned validator sets could be identical or even different but with the same sequence number; in this case, resolving the discrepancy is a job for the consensus algorithm. Account numbers could only help here in the edge case that the split chains assign different account numbers to the same address/public key, but would only be a small potential help in de-conflicting temporary splits and doesn't relate to replay protection.

@moul is correct that the account number can be a failsafe against running the same gnokey command on different networks, but I wouldn't consider this a significant enough safety measure to make a decision about account numbers way or the other.