BNS upgrade procedure

[jcnelson]

It has come to our attention that the BNS name-renewal function has at least two bugs:

• Name renewals are accidentally free -- the code doesn't charge the user for the cost of the name
• If given a new owner as an optional argument, the code won't transfer the name -- it'll be ignored.

The latter bug can be worked around with a name-transfer, but the former is concerning. We don't want squatters.

Fixing this is a consensus-breaking task, so it'll have to be done as part of Stacks 2.1. I think the least-risky way to do this is to simply create a BNS "wrapper contract" called .bns-2.1, which wraps all the define-public BNS methods with stubs that simply run a contract-call? to the corresponding BNS method. For name-renewal specifically, .bns-2.1 will first call the original name-renewal contract, and if successful, will then burn the requisite renewal fee and run a name-transfer. We'd do contract-call interception to make it so that all calls to define-public methods in .bns will get rerouted to .bns-2.1. So, .bns would be put into "read-only" mode, and .bns-2.1 would in most cases simply do whatever .bns does.

So, .bns-2.1 would look like the following:

(define-public (name-preorder (hashed-salted-fqn (buff 20))
                              (stx-to-burn uint))
  (contract-call? .bns name-preorder hashed-salted-fqn stx-to-burn))

(define-public (name-register (namespace (buff 20))
                              (name (buff 48))
                              (salt (buff 20))
                              (zonefile-hash (buff 20)))
  (contract-call? .bns name-register namespace name salt zonefile-hash))
  
;; and so on and so forth

(define-public (name-renewal (namespace (buff 20))
                             (name (buff 48))
                             (stx-to-burn uint)
                             (new-owner-opt (optional principal))
                             (zonefile-hash-opt (optional (buff 20))))
  ;; There's a bug in .bns that causes it to ignore `new-owner-opt`, so don't pass it here.
  (match (contract-call? .bns name-renewal namespace name stx-to-burn none zonefile-hash-opt))
    (ok ok-case
      (begin
        ;; There's a bug in .bns that causes it to *not* burn the STX, so do so here.
        ;; This guaranteed to succeed since name-renewal checks tx-sender's STX balance.
        (unwrap-panic (stx-burn? stx-to-burn tx-sender))
        (match new-owner-opt
          new-owner
            (contract-call? .bns name-transfer namespace name new-owner zonefile-hash-opt)
          (ok ok-case))))
    (err err-case
      (err err-case)))
)

We'd update the cost functions for these methods so that the cost to load the contract in the inner contract-call? gets ignored, and we'd make it so the new name-renewal method takes the same runtime as the old name-renewal.

What do you all think about this?

cc @kantai @lgalabru @pavitthrap @reedrosenbluth @zone117x @gregorycoppola

[friedger]

What are the concerns about fixing the contract directly?

[jcnelson]

The system isn't designed with the expectation that the contract code can be replaced. A lot of things would break if you replaced a function's code body. For example, all the analysis metadata for all contracts that call into the replaced contract would be invalidated.

But, the VM does support special-case handling of contract-call?s (as well as ContractCall transactions). It currently must do so in order to handle PoX stacking. This proposal would do something similar for .bns -- any contract-call to .bns after 2.1 would be redirected to the .bns-2.1 contract.

[jcnelson]

Let me be explicit about this point: I am not suggesting we replace the existing BNS contract. I do not think it is ever going to be acceptable or even safe to try and change code or data that is already deployed. After all, the point of having a blockchain run programs is to ensure that those programs remain deployed as-is (for better or worse).

This proposal is concerned instead with wrapping an existing contract with a new contract. No modifications to the code or data will be carried out. In fact, this fix does not require a hard fork -- it could be shipped in 2.0. It could be deployed as a soft-fork, where if adopted, miners would never build on blocks that contained direct contract-calls to the old BNS contract.