Feat/get pox addrs #3245

jcnelson · 2022-08-09T20:38:34Z

This PR implements #3155 by adding get-burn-block-info? pox-addrs <burn-height> to Clarity. It returns an optional tuple with two items:

addrs: The list of up to two PoX addresses (type (list 2 { hashbytes: (buff 20), version: (buff 1) }))
payout: The number of burnchain tokens (i.e. satoshis) paid to each PoX address across all block-commits (type uint).

It returns (some ..) when the sortition for burn-height is in the transaction's Stacks block's fork, and none if not.

~~Burn addresses are not reported. This method will return an empty addrs list during the reward phase if the BTC are to be burnt, and will return an empty addrs list during the prepare phase.~~ EDIT: Burn addresses are reported -- up to two during the reward phase for unclaimed PoX reward slots, and one during the prepare phase.

The payout value is per TX output. During the reward phase, there are two outputs whose values are equal to half of the node's burn_fee_cap. This is true even if there are no more PoX reward addresses to pay. During the prepare phase, there is one output, and its value is equal to the node's burn_fee_cap. So for example if the network is spending 10,000 sats per block-commit, then payout will be 5,000 during the reward phase and 10,000 during the prepare phase.

In addition to implementing this feature, this PR replaces the use of StacksAddress to represent Bitcoin transaction outputs with a similar PoxAddress structure. This structure captures all of the fields in a pox-addr argument to stack-stx or delegate-stack-stx, which is not recoverable from just the Bitcoin chain state. It is these values that are passed back to the caller from get-burn-block-info? pox-addrs.

The addition of PoxAddress is not only necessary to represent the full PoX address data, but also lays the groundwork for implementing segwit (and taproot) addresses. These addresses cannot be represented in a StacksAddress, but because PoxAddress is an enum, we can later add type variants that represent any scriptPubKey we want.

… the .pox or .pox-2 contracts, and add a trait method for getting the PoX addresses and payouts sent in a particular burn block height on the calling Stacks fork

…method for pulling the requisite information out of the BurnStateDB impl and converts it into a Clarity value)

…echecker

…hain recipients; also, make prepare phase calculation a static method

…sses for this sortition as well as the per-output payout (which will then be reported via BurnStateDB implementations by querying the MARF)

…tacksAddress to represent PoX reward addresses

…ase58 strings and Bitcoin txouts

… other tests to use PoxAddress instead of StacksAddress)

…put to `stack-stx` from on-chain StackStxOp

…ithout uncommitted state from the underlying storage connection

…torage transaction state with only an immutable reference (required to make it possible to query the sortition DB MARF within a BurnStateDB impl)

…ed writes are not visible); instead, pass an immutable reference to the tx's db handle

kantai · 2022-08-10T16:45:20Z

clarity/src/vm/database/clarity_db.rs

+pub enum PoxAddress {
+    /// represents { version: (buff u1), hashbytes: (buff 20) }.
+    /// The address hash mode is optional because if we decode a legacy bitcoin address, we won't
+    /// be able to determine the hash mode since we can't distinguish segwit-p2sh from p2sh
+    Standard(StacksAddress, Option<AddressHashMode>),
+}


I think this struct should be in the chainstate module, rather than in the Clarity module. I know there's some leakage of PoX into the Clarity module (mostly in the function signatures in the database), but most of these methods seem more important to the chainstate than it is to Clarity -- all Clarity really needs is the tuples:

/// Returns the paid out PoX addresses (as Clarity tuples of type `{ hashbytes: (buff 1), version: (buff 20) }` ) pub fn get_pox_payout_addrs_for_burnchain_height( &mut self, burnchain_block_height: u32, ) -> Option<(Vec<TupleData>, u128)> {

Whereas the chainstate module actually uses most of the methods of this struct.

Sure, I can do that

Done: 49dea6e

…ve BurnStateDB::get_pox_payout_addrs() return Option<Vec<TupleData>, u128> instead of Option<Vec<PoxAddress>, u128>. This makes more sense since the chainstate module depends on PoxAddress far more so than Clarity. So, have the BurnStateDB return the tuple representation of a PoxAddress, and have the chainstate module convert that tuple into a PoxAddress once needed.

kantai · 2022-08-11T18:31:36Z

src/chainstate/burn/db/sortdb.rs

+    pub fn pox_reward_set_payouts_key() -> String {
+        format!("sortition_db::reward_set::payouts")
+    }
+
+    pub fn pox_reward_set_payouts_value(addrs: Vec<PoxAddress>, payout_per_addr: u128) -> String {
+        serde_json::to_string(&(addrs, payout_per_addr)).unwrap()
+    }
+
+    pub fn pox_reward_set_payouts_decode(addr_str: &str) -> (Vec<PoxAddress>, u128) {
+        let addrs_and_payout: (Vec<PoxAddress>, u128) = serde_json::from_str(addr_str).unwrap();
+        addrs_and_payout
+    }


Does this value need to be stored in the MARF, or can it just be indexed by the SortitionID in a sqlite table?

As in, lookups are always of the form what was the payout set at <sortition-id> (which doesn't need to traverse the sortition history), and they're never what was the payout set at <sortition-id> or if there wasn't one, what was the last payout prior? (which could only be answered through parent traversal).

I suppose it doesn't need to be written to the MARF. I can just write it to a DB table -- it would make the I/O faster at least.

kantai

This looks great to me, @jcnelson, but I think it may not be necessary to store the PoX payout information in the sortition MARF, but it could instead just be an entry in a sortition-id indexed table. It's up to you if you think the difference would be substantial enough to merit changes.

…, since we'll have an entry for every sortition

jcnelson · 2022-08-13T16:28:11Z

I think I might want to update the API for this to return the burn addresses as well (so then it's obvious how much total BTC was spent, given the per-output spend)

jcnelson · 2022-08-15T15:32:28Z

Need to make the return type easier to understand:

Return burn addresses explicitly
Return burn addresses in the prepare phase, so that the number of entries in the list reflects what block-commits are actually doing (so it's easier to interpret the payout field)

jcnelson · 2022-08-16T14:27:59Z

Okay, the above is done. cc @kantai @gregorycoppola @pavitthrap for review/approval.

…TS_PER_COMMIT (in the reward phase) or 1 (in the prepare phase)

…nd at the right time

… reported, and at the right time

kantai · 2022-08-17T21:32:50Z

src/chainstate/burn/operations/leader_key_register.rs

+        let address = outputs[0]
+            .address
+            .clone()
+            .try_into_stacks_address()
+            .ok_or_else(|| {
+                test_debug!("Invalid address: not representable as a StacksAddress");
+                op_error::InvalidInput
+            })?;


Is this a consensus change?

Nope. First, .address isn't even used in LeaderKeyRegister. I'm even going so far as to remove it in my segwit PR. Second, .try_into_stacks_address() is infallible right now, since the only PoxAddress variant (Standard(addr)) contains a StacksAddress.

In that case, this should panic -- as it is currently written, it suggests that an error would invalidate the LeaderKeyRegister (similarly for all the operations that this try_into_stacks_address() method is invoked on).

The reason it's written this way is because .try_into_stacks_address() will soon be fallible, i.e. on the arrival of the native segwit support. The reason it will be fallible is because there will not be a way to convert a segwit address into a principal type, so parsing StackStxOp and TransferStxOp must fail here if the intended recipient can't be represented as a principal.

kantai · 2022-08-17T21:38:45Z

src/chainstate/stacks/address.rs

+    pub fn bytes(&self) -> Vec<u8> {
+        match *self {
+            PoxAddress::Standard(addr, _) => addr.bytes.0.to_vec(),
+        }
    }


I think it's better to return a &[u8] here. The caller can then decide if they want to copy to a vector or not. This gets used for sorting, so requiring a bunch of copies would be inefficient there.

If you're referring for StacksChainState::make_reward_set(), the problem with doing this the way you suggest is that the reference &addrs.bytes.0 doesn't live long enough in the closure passed to .sort_by_key(). Maybe we should keep this as-is and use .sort_by_cached_key()?

I guess that's fine to leave as is then.

kantai · 2022-08-17T21:39:35Z

src/chainstate/stacks/address.rs

+/// A PoX address as seen by the .pox and .pox-2 contracts.
+/// Used by the sortition DB and chains coordinator to extract addresses from the PoX contract to
+/// build the reward set and to validate block-commits.


Maybe add "This comprises a larger set of possible addresses than StacksAddress"?

Sure, will do

kantai · 2022-08-17T21:40:47Z

src/chainstate/stacks/address.rs

+    pub fn hashmode(&self) -> Option<AddressHashMode> {
+        match *self {
+            PoxAddress::Standard(_, hm) => hm.clone(),
+        }
+    }
+
+    #[cfg(any(test, feature = "testing"))]
+    pub fn version(&self) -> u8 {
+        self.hashmode()
+            .expect("FATAL: tried to load the hashmode of a PoxAddress which has none known")
+            as u8
+    }
+
+    pub fn bytes(&self) -> Vec<u8> {
+        match *self {
+            PoxAddress::Standard(addr, _) => addr.bytes.0.to_vec(),
+        }
    }


Can you add rustdocs to the public functions of this struct? I think as it abstracts multiple kinds of addresses, functions like "bytes" are less self-explanatory.

Sure, will do

kantai

This looks good to me, I think there's just some superficial comments remaining: slight tweaks to an interface and some additional rustdocs.

pavitthrap

lgtm

pavitthrap · 2022-08-18T16:09:44Z

src/chainstate/stacks/address.rs

+        }
+    }
+
+    /*


remove dead code

blockstack-devops · 2024-11-17T00:22:47Z

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

jcnelson added 30 commits August 9, 2022 14:38

feat: add PoxAddress type to represent a pox-addr tuple passed into…

bf54779

… the .pox or .pox-2 contracts, and add a trait method for getting the PoX addresses and payouts sent in a particular burn block height on the calling Stacks fork

chore: expose PoxAddress

b63f764

feat: add docs for get-burn-block-info? pox-addrs

76ba303

feat: implement get-burn-block-info? pox-addrs (wraps the database …

6eefc51

…method for pulling the requisite information out of the BurnStateDB impl and converts it into a Clarity value)

chore: expose PoxAddress as clarity::vm::PoxAddress

da14380

chore: mock get_pox_payout_addrs()

a1000b5

chore: add type signature for get-burn-block-info? pox-addrs to typ…

f2bd018

…echecker

chore: use PoxAddress instead of StacksAddress for representing burnc…

dc0aaf1

…hain recipients; also, make prepare phase calculation a static method

chore: use PoxAddress instead of StacksAddress

8d973d1

chore: use PoxAddress instead of StacksAddress

7914f7b

chore: add FromColumn<PoxAddress>

e4d48e9

chore: log parent block-commit coordinates when rejecting a block-commit

8900410

feat: when updating the sortition DB MARF, store the PoX payout addre…

da96474

…sses for this sortition as well as the per-output payout (which will then be reported via BurnStateDB implementations by querying the MARF)

refactor: commit_outs are now PoxAddress, not StacksAddress

3bae932

refactor: burnchain recipient is a PoxAddress now

8a9e45b

chore: StackStxOp and LeaderBlockCommitOp use PoxAddress instead of S…

5fe5a8d

…tacksAddress to represent PoX reward addresses

chore: use PoxAddress instead of StacksAddress for recipient

76b888d

refactor: burnchain recipient is now a PoxAddress

e80dcde

refactor: burnchain recipient is now a PoxAddress

6db3c90

chore: PoX reward set now uses PoxAddress, not StacksAddress

d28423f

refactor: use PoxAddress instead of StacksAddress

dd214fb

feat: add PoxAddressExtensions trait for converting a PoxAddress to b…

27a85b5

…ase58 strings and Bitcoin txouts

chore: add get_pox_payout_addrs()

fdb4e9c

refactor: use PoxAddress instead of StacksAddress

6f92edd

feat: add unit tests for get-burn-block-info? pox-addrs (and update…

c7cf376

… other tests to use PoxAddress instead of StacksAddress)

refactor: now using PoxAddress instead of StacksAddress to recover in…

a824c26

…put to `stack-stx` from on-chain StackStxOp

chore: mock get_pox_payout_addrs()

78782d1

chore: TrieCache and TrieCacheState are now clone()-able

3635d41

refactor: distinguish between reopening a MARF as read-only with or w…

85ea09a

…ithout uncommitted state from the underlying storage connection

feat: add the ability to run a closure against the uncommitted MARF s…

b48f618

…torage transaction state with only an immutable reference (required to make it possible to query the sortition DB MARF within a BurnStateDB impl)

jcnelson added 3 commits August 10, 2022 11:47

fix: don't re-open a storage tx's underlying DB (since then uncommitt…

a529500

…ed writes are not visible); instead, pass an immutable reference to the tx's db handle

docs: note that uncommitted writes are visible in get_indexed()

5742e37

chore: log initial mining bonus

d3716f3

kantai reviewed Aug 10, 2022

View reviewed changes

kantai reviewed Aug 11, 2022

View reviewed changes

jcnelson added 4 commits August 12, 2022 12:34

chore: revert changes to the MARF's read-only views (because YAGNI)

eb898ea

chore: return Clarity tuples not PoxAddresses

2bb1c1d

chore: store PoX payouts in the snapshots table instead of the MARF…

c9a3d03

…, since we'll have an entry for every sortition

feat: query pox payouts from snapshots table, not the MARF

699a3c4

jcnelson added 4 commits August 16, 2022 10:29

docs: indicate that burn addresses will be reported in pox-addrs

e0b342c

fix: populate PoX addresses paid out with burn addresses, up to OUTPU…

09ad07a

…TS_PER_COMMIT (in the reward phase) or 1 (in the prepare phase)

fix: update test to verify that 1 or 2 burn addresses are reported, a…

0a49e0b

…nd at the right time

fix: update integration test to verify that 1 or 2 burn addresses get…

4d78bfc

… reported, and at the right time

kantai reviewed Aug 17, 2022

View reviewed changes

kantai approved these changes Aug 17, 2022

View reviewed changes

pavitthrap approved these changes Aug 18, 2022

View reviewed changes

chore: add/expand rust docs and remove dead code

7f5af6b

jcnelson merged commit 357405b into next Aug 22, 2022

zone117x mentioned this pull request Aug 29, 2022

[Stacks 2.1] Support Stacks 2.1 in @stacks/stacking (PoX-2) hirosystems/stacks.js#1347

Closed

4 tasks

blockstack-devops added the locked label Nov 17, 2024

stacks-network locked as resolved and limited conversation to collaborators Nov 17, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feat/get pox addrs #3245

Feat/get pox addrs #3245

jcnelson commented Aug 9, 2022 •

edited

Loading

kantai Aug 10, 2022

jcnelson Aug 11, 2022

jcnelson Aug 11, 2022

kantai Aug 11, 2022

kantai Aug 11, 2022 •

edited

Loading

jcnelson Aug 11, 2022

jcnelson Aug 12, 2022

kantai left a comment

jcnelson commented Aug 13, 2022

jcnelson commented Aug 15, 2022

jcnelson commented Aug 16, 2022

kantai Aug 17, 2022

jcnelson Aug 17, 2022

kantai Aug 18, 2022

jcnelson Aug 19, 2022

kantai Aug 17, 2022

jcnelson Aug 17, 2022

kantai Aug 18, 2022

kantai Aug 17, 2022

jcnelson Aug 19, 2022

jcnelson Aug 19, 2022

kantai Aug 17, 2022

jcnelson Aug 19, 2022

jcnelson Aug 19, 2022

kantai left a comment

pavitthrap left a comment

pavitthrap Aug 18, 2022

jcnelson Aug 19, 2022

blockstack-devops commented Nov 17, 2024

+                      }
+                  }
+                  /*

Feat/get pox addrs #3245

Feat/get pox addrs #3245

Conversation

jcnelson commented Aug 9, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kantai Aug 11, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kantai left a comment

Choose a reason for hiding this comment

jcnelson commented Aug 13, 2022

jcnelson commented Aug 15, 2022

jcnelson commented Aug 16, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kantai left a comment

Choose a reason for hiding this comment

pavitthrap left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blockstack-devops commented Nov 17, 2024

jcnelson commented Aug 9, 2022 •

edited

Loading

kantai Aug 11, 2022 •

edited

Loading