[Stacks 2.1] Enforce validate `pox-addr` in `delegate-stx`

[zone117x]

A two year old TODO comment/question is in both the original pox contract and pox-2 contract source:

does the delegate need to use a specific pox recipient address? https://github.com/stacks-network/stacks-blockchain/blob/34654dee5d2f453c8ad21bbcfde94c4ec731568c/src/chainstate/stacks/boot/pox-2.clar#L96-L106

I think the answer is: no, this is essentially scratch space that is delegate/client/pool specific which typically corresponds to a valid STX and/or bitcoin address, but does not have to.

Is this correct? Either way, can we clarify this?

EDIT: @jcnelson clarified that delegate-stx should be provided with a valid pox-addr (or none), and that if an invalid address is given, then the pool operator will not be able to perform the later call to delegate-stack-stx.

And also that the code comment isn't a dev question/TODO, but rather a convention of describing the values in the form of a question. That makes a lot more sense now, but the comment pair with the surprising behavior of delegate-stx led to confusion.

IMO the delegate-stx fn should be updated to require a valid pox-addr (it appears to be the only one with this issue?). And/or potentially updating the fn comment/doc to make users aware of this scenario.

[jcnelson]

That's not a TODO. The word "need" is speaking to whether or not the pool operator must use the stacker's given PoX address. If the PoX address is some, then they "need" to use it. If it's none, then they don't -- they can just use whatever PoX address they want.

[zone117x]

@jcnelson sounds like 2 different subjects covered in your response:

That's not a TODO

While "TODO" is not in the comment, the comment "does the delegate need to use a specific pox recipient address?" certainly looks like a question from the developers along the lines of "what does/should this do?"

The word "need" is speaking to whether or not the pool operator must use the stacker's given PoX address. If the PoX address is some, then they "need" to use it. If it's none, then they don't

Is this convention or is this enforced? IIRC last I checked that may be convention, but is not enforced. I just tested a transaction with a definitely-incorrect (not btc/stx encodable) pox-address:

const poxAddr = {
  hashbytes: bufferCV(Buffer.from('send to matt@paypal!')),
  version: bufferCV(Buffer.from([0xff])),
};
const delegateStxTx = await makeContractCall({
  senderKey: delegateeAccount.secretKey,
  contractAddress,
  contractName,
  functionName: 'delegate-stx',
  functionArgs: [
    uintCV(delegateAmount),
    standardPrincipalCV(delegatorAccount.stxAddr),
    noneCV(), // untilBurnBlockHeight
    someCV(tupleCV(poxAddr)),
  ]
});

I think need could be used to describe the pox-addr value, but "application-specific scratch space" sounds much more accurate IMO.

Regardless, the "what's going on?" code comment in a boot contract should, at a minimum, be removed / revised to be more helpful and confidence inspiring. In an ideal world where backwards-compat isn't an issue, this value would be enforced as a valid stx and/or bitcoin addr (or instead called scratch-space / memo).

[jcnelson]

You misunderstand. When a user stacks through a pool, they have the option of requesting that the pool operator use a particular PoX address when they call delegate-stack-stx. If the user's delegation record's pox-addr is set to (some ...), then the pox-addr passed to delegate-stack-stx must match it in order for the operation to proceed. In addition, the pox-addr passed to delegate-stack-stx must be a valid PoX address.

If the user passes an invalid PoX address into delegate-stx, all they succeed in doing is ensuring that the pool operator cannot call delegate-stack-stx on their behalf.

We can make it so that the user's call to delegate-stx checks the PoX address for validity, but that's a separate concern. The behavior of pox-addr in delegation-state is well-known.

[zone117x]

Got it, good know know the delegate-stack-stx function checks for a valid address.

If the user passes an invalid PoX address into delegate-stx, all they succeed in doing is ensuring that the pool operator cannot call delegate-stack-stx on their behalf ... The behavior of pox-addr in delegation-state is well-known.

FWIW it was not well-known to me, and was not well-known to others as evidence in the invalid values passed to this function historically (it's been a while since I've scanned for those but I could try again if necessary). It also wasn't super obvious from reading the contract source (especially with the confusing code comment).

[zone117x]

Based on @jcnelson's feedback, I think the call to action here could be: enforce validate address in delegate-stx fn, and/or update the fn comment to indicate the error condition that @jcnelson outlined above for usage around invalid pox-addr.

[jcnelson]

FWIW it was not well-known to me

Sorry, that sounded snotty now that I read it again. I was only trying to say that it's well-known that users can request a specific PoX address for the delegate to use when stacking on their behalf (e.g. it's in the spec, and pool operators use it).

I'm in favor of fixing delegate-stx to require a supported PoX address in pox-2.

[zone117x]

No worries! I agree with fixing delegate-stx -- the error condition you described made the whole thing click for me. It would definitely be a worth while developer experience improvement.

[jcnelson]

Merged!