Feat/btc stx exchange rate #3142

jcnelson · 2022-05-19T17:44:48Z

This PR addresses #3043 by adding the following new fields to get-block-info? in Stacks 2.1:

block-reward: returns the total number of uSTX awarded to the miner once the block reward matures.
miner-spend-total: returns the total number of satoshis spent by all Stacks miners trying to win this block.
miner-spend-winner: returns the number of satoshis spent by the winning miner on this block.

The block-reward value is only (some ..) after the queried block receives MINER_REWARD_MATURITY + 1 confirmations. This is because the block reward can be slashed within the first MINER_REWARD_MATURITY blocks by a PoisonMicroblock transaction, should the miner publish two or more conflicting microblock streams during their tenure. Therefore, the system cannot reliably report the block reward until it is no longer possible to publish a PoisonMicroblock transaction.

The Stacks chainstate is not backwards-compatible with this PR. Specifically:

In order for this PR to faithfully report the block-reward, I had to make it so that the system stores the MinerReward structs that mature once a block is appended to the chainstate. The actual number of tokens credited to the miner isn't recorded anywhere; we just directly pay to the miner's account when processing matured rewards.
In order to get miner-spend-total to work, I had to fix [chainstate] burnchain_sortition_burn calculation is for the previous, not current sortition #2492 to store the right sortition burn for a Stacks block. While I don't believe this is a consensus-breaking change, since we don't appear to use the value anywhere, the fact that this value is different than what we have now means that we're gonna need to recalculate it.

I could go back and add schema migration code for these two things, but I'm hesitant to do so, since the time taken to write, test, and review the migration code will likely exceed the time to just boot up a 2.1 node from genesis.

…ock info properties

… spent by the winning block, burnchain tokens spent by all miners in a block, and the number of uSTX awarded to the miner in a block once they mature

…tokens spent by miners and get the block reward

…y2 for querying the btc and stx exchange rate

…he matured block rewards are for each block. This data gets fed into `get-block-info?`. We store them on maturity, not on creation, so we can account for PoisonMicroblocks getting processed before the maturity window

…ich *shouldn't* affect consensus, since we don't use it anywhere, *but we will need to test it nevertheless!*). Also, plumb through matured miner rewards from block-processing so we can store them

…ge is *not* backwards-compatible! Stacks 2.1 nodes will need to boot from genesis if this change is merged.

…ds per block. N.b. the number of STX awarded isn't tested yet due to #3140, but will be fixed once #3140 is addressed

…te DB.

…get at it

jcnelson · 2022-05-19T17:47:00Z

In writing this code, I discovered a consensus bug (#3140) in how we calculate microblock transaction fees. I'm going to send another PR on top of this one that fixes it. I documented where this bug occurs in the comments in this PR.

codecov · 2022-05-19T18:13:21Z

Codecov Report

Merging #3142 (00ca64b) into next (43d8fb5) will increase coverage by 0.19%.
The diff coverage is 92.15%.

@@            Coverage Diff             @@
##             next    #3142      +/-   ##
==========================================
+ Coverage   84.00%   84.19%   +0.19%     
==========================================
  Files         268      268              
  Lines      210798   212573    +1775     
==========================================
+ Hits       177077   178981    +1904     
+ Misses      33721    33592     -129

Impacted Files	Coverage Δ
clarity/src/vm/docs/mod.rs	`86.30% <0.00%> (-1.99%)`	⬇️
clarity/src/vm/test_util/mod.rs	`56.42% <0.00%> (-5.30%)`	⬇️
src/chainstate/stacks/mod.rs	`75.03% <ø> (ø)`
src/clarity_cli.rs	`75.90% <0.00%> (-0.54%)`	⬇️
stacks-common/src/libcommon.rs	`100.00% <ø> (ø)`
src/clarity_vm/database/mod.rs	`83.94% <56.81%> (-4.69%)`	⬇️
src/chainstate/stacks/db/accounts.rs	`90.03% <77.25%> (-2.21%)`	⬇️
src/chainstate/stacks/db/mod.rs	`87.26% <81.25%> (-0.13%)`	⬇️
clarity/src/vm/database/clarity_db.rs	`89.98% <81.53%> (-0.43%)`	⬇️
src/chainstate/stacks/db/blocks.rs	`89.53% <90.00%> (+0.01%)`	⬆️
... and 46 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1c7db10...00ca64b. Read the comment docs.

…nt's block's evaluated epoch is 2.1, we start using the child block's reported streamed tx fees to determine the parent's reward. We need to use the parent block's evaluated epoch, since a stream cannot cross an epoch boundary.

…eward's parent block's evaluated epoch

…fee behavior, the new fee behavior (by way of the new get-block-info? fields), and the transition gating

…om two or more forks that share the same ancestor block whose reward is about to mature

gregorycoppola

Thanks for the change.. It's making sense to me but had a few questions.

gregorycoppola · 2022-05-20T18:12:49Z

src/chainstate/stacks/db/accounts.rs

@@ -474,6 +563,198 @@ impl StacksChainState {
        Ok(())
    }

+    /// Store a matured miner reward for subsequent query in Clarity, without doing any validation
+    fn inner_insert_matured_miner_payment<'a>(


inner_insert_matured_miner_reward is more in keeping with the naming convention, i think

I'm deliberately distinguishing between rewards (which are to be paid) and payments (which are already paid).

Not sure if this distinction is readily understandable. Maybe add a comment with this intended meaning.

Alternatively, you could use rewards / past rewards or payments / future payments

I agree with the above, payment vs. reward isn't a readily understandable distinction here -- isn't matured already indicating that the reward has been paid? (Also, the code seems to ignore this distinction between "payment" and "reward", as the 4th argument is called reward and not payment).

src/clarity_vm/tests/contracts.rs

src/chainstate/stacks/miner.rs

src/clarity_vm/tests/contracts.rs

gregorycoppola · 2022-05-20T20:53:20Z

clarity/src/vm/analysis/type_checker/natives/mod.rs

-        BlockInfoProperty::lookup_by_name(block_info_prop_str).ok_or(CheckError::new(
-            CheckErrors::NoSuchBlockInfoProperty(block_info_prop_str.to_string()),
-        ))?;
+        BlockInfoProperty::lookup_by_name_at_version(block_info_prop_str, &checker.clarity_version)


regarding this:

I could go back and add schema migration code for these two things, but I'm hesitant to do so, since the time taken to write, test, and review the migration code will likely exceed the time to just boot up a 2.1 node from genesis.

Are you comparing the time of the miners to your time? I mean it's fine to make this tradeoff, but not for the reason that the users' time is 1:1 substitutable with yours.

It takes less than 30 hours to boot from genesis in the latest release. This is something you could kick off on a Friday and expect to be done on a Monday.

More generally, I think we want to get folks into the habit of spawning a node from genesis on each hard fork. It would:

reduce the codebase's technical debt by giving us a chance to remove the last epoch's schema migrations

reduce the testing burden of hard forks by only requiring us to verify that the new node can boot from genesis, as opposed to booting from all of the previously-supported epochs' chainstates.

de-risk the chance that a bug in the ever-growing sequence of schema migrations leads to a chain split

enable us to be able to make significant, principled, and even green-field changes to the database schemas without worrying about compatibility, thereby reducing development, review, and testing time for deploying them

I think it's fair to consider each new epoch of Stacks to be a wholly new blockchain, whose genesis state just happens to be derived from the last epoch's chainstate. In fact, I'd be supportive of shipping each hard fork with all of the blocks from the last epochs as a database, so they don't even need to be downloaded on boot-up (just re-processed and re-validated against the Bitcoin blockchain). This would certainly speed up the boot-up process.

…confirm that we have 0 tx fees from them stored in the MinerReward structs

…s-network/stacks-blockchain into feat/btc-stx-exchange-rate

pavitthrap · 2022-05-24T15:52:15Z

clarity/src/vm/analysis/type_checker/tests/mod.rs

@@ -252,10 +266,25 @@ fn test_get_block_info() {
            &format!("{}", type_check_helper(&good_test).unwrap())
        );
    }
+    for (good_test_v210, expected_v210) in good.iter().zip(expected.iter()) {


Need to also iterate over good_v10 & expected_v10 in the clarity v2 context.

Also maybe want to run normal good/bad tests with the function type_check_helper_v1.

pavitthrap · 2022-05-24T16:21:15Z

clarity/src/vm/database/clarity_db.rs

@@ -272,6 +285,18 @@ impl HeadersDB for NullHeadersDB {
    fn get_miner_address(&self, _id_bhh: &StacksBlockId) -> Option<StacksAddress> {
        None
    }
+    fn get_burnchain_tokens_spent_for_block(&self, _id_bhh: &StacksBlockId) -> Option<u128> {
+        None


Just curious as to why we are returning None in most of the methods in this trait implementation here.
More specifically, is this a place we could have better trait design? For example, if a method takes in a type that must implement the BurnStateDB trait, would we instead want to break that down into separate traits / use supertraits?

This implementation is meant to represent an empty database, so there is no state to return. If you need a unit test that relies on specific state returned for specific blocks, you'd just add a new implementation. We do this for HeadersDB already in several places.

pavitthrap · 2022-05-24T16:52:35Z

clarity/src/vm/docs/mod.rs

+Stacks block at the given Stacks chain height.
+
+`id-header-hash`: This property returns a `(buff 32)` value containing the _index block hash_ of a Stacks block.   This hash is globally unique, and is derived
+from the block hash and the history of accepted PoX operations.  This is also the block hash value you would pass into `(at-block)`.


"the history of accepted PoX operations" - I think this phrasing could be clearer to make this explanation more accessible

The alternative is to just say consensus_hash, which is at least as hard to explain (since it is derived from the history of PoX operations).

pavitthrap · 2022-05-24T17:09:13Z

src/chainstate/stacks/db/accounts.rs

@@ -474,6 +563,198 @@ impl StacksChainState {
        Ok(())
    }

+    /// Store a matured miner reward for subsequent query in Clarity, without doing any validation
+    fn inner_insert_matured_miner_payment<'a>(


Not sure if this distinction is readily understandable. Maybe add a comment with this intended meaning.

Alternatively, you could use rewards / past rewards or payments / future payments

pavitthrap · 2022-05-24T17:49:23Z

src/chainstate/stacks/db/mod.rs

+        mature_miner_payouts: Option<(MinerReward, Vec<MinerReward>, MinerReward)>, // (miner, [users], parent)
+        mature_rewards_info: Option<MinerRewardInfo>,


Could make this a struct -
would eliminate the panic here: let reward_info = mature_rewards_info.expect("FATAL: have mature payouts but no rewards info");

testnet/stacks-node/src/tests/neon_integrations.rs

…ratuitous .expect()

…lock fee reporting (not a consensus bug since user burn supports are disabled)

kantai · 2022-06-01T20:24:02Z

src/chainstate/stacks/miner.rs

+    #[test]
+    fn test_get_block_info_v210() {


Between this PR and #3144, there's almost 2,000 new lines of tests in this file, and these tests are really only sort of related to the miner module. Only about 2,000 lines of this 10k+ line file are implementation. Both of these things strongly suggest to me that the tests components of this file should be split out (i.e., the fact that tests are being added to this file without any changes otherwise in this module, and the fact that this file is now huge and almost entirely composed of tests). It doesn't have to happen in this PR, but I definitely believe it needs to happen.

Sure, I'll open an issue.

kantai · 2022-06-01T20:27:48Z

src/chainstate/stacks/db/accounts.rs

+            &format!("{}", &reward.coinbase),
+            &format!("{}", &reward.tx_fees_anchored),
+            &format!("{}", &reward.tx_fees_streamed_confirmed),
+            &format!("{}", &reward.tx_fees_streamed_produced),


x.to_string() is always the same as format!("{}", x)

Suggested change

&format!("{}", &reward.coinbase),

&format!("{}", &reward.tx_fees_anchored),

&format!("{}", &reward.tx_fees_streamed_confirmed),

&format!("{}", &reward.tx_fees_streamed_produced),

&reward.coinbase.to_string(),

&reward.tx_fees_anchored.to_string(),

&reward.tx_fees_streamed_confirmed.to_string(),

&reward.tx_fees_streamed_produced.to_string(),

kantai · 2022-06-01T20:34:15Z

src/chainstate/stacks/db/accounts.rs

+            == &StacksBlockHeader::make_index_block_hash(
+                &FIRST_BURNCHAIN_CONSENSUS_HASH,
+                &FIRST_STACKS_BLOCK_HASH,
+            )


StacksBlockId::new is a more succinct way to create StacksBlockIds, and it conveys the type more clearly. I don't feel very strongly about actually enforcing this, but it's probably better practice to stick to one in our codebase.

I kinda feel like we should just remove all instances of StacksBlockHeader::make_index_block_hash() in one fell swoop in a separate PR. There's lots of them. What do you think?

Yeah, I'm fine with that, but I'd prefer not to add new ones in the mean time

kantai

This all looks good to me, just some superficial comments.

gregorycoppola · 2022-06-01T21:37:32Z

LGTM, but the other reviewers have left comments so I'll let them approve.

jcnelson · 2022-06-06T18:00:38Z

I'm ready for another pass @kantai @gregorycoppola @pavitthrap.

…d_miner_reward

kantai

LGTM

gregorycoppola

Assuming tests pass

Fix/microblock fees

jcnelson added 17 commits May 19, 2022 13:15

feat: lookup block info properties by name and version

e94ccfb

feat: unit tests for type-checking get-block-info? with Clarity2 bl…

f8bb42c

…ock info properties

feat: expand HeadersDB to include methods to get the burnchain tokens…

c75906f

… spent by the winning block, burnchain tokens spent by all miners in a block, and the number of uSTX awarded to the miner in a block once they mature

chore: API sync

ecdc1ba

feat: add support for new block info properties to get the burnchain …

f93aaa3

…tokens spent by miners and get the block reward

chore: API sync

a20d1fc

feat: add new block info properties that are only available in Clarit…

67db526

…y2 for querying the btc and stx exchange rate

chore: API sync

eb084c0

fix: fix #2492 by using the burnchain tip's burnchain token spend (wh…

6e6b095

…ich *shouldn't* affect consensus, since we don't use it anywhere, *but we will need to test it nevertheless!*). Also, plumb through matured miner rewards from block-processing so we can store them

feat: store matured miner rewards as they mature. Note that this chan…

3e0aab2

…ge is *not* backwards-compatible! Stacks 2.1 nodes will need to boot from genesis if this change is merged.

chore: test new get-block-info? fields for btc and stx spends/rewar…

cb6960d

…ds per block. N.b. the number of STX awarded isn't tested yet due to #3140, but will be fixed once #3140 is addressed

chore: API sync

577a7a2

feat: plumb through new get-block-info keywords to the StacksChainSta…

f57ee6f

…te DB.

chore: unit tests on new get-block-info? keywords

9455d19

refactor: move the miner block reward maturity window so clarity can …

960ce54

…get at it

chore: update get-block-info? documentation with new stacks 2.1 fields

2e5c42f

jcnelson requested review from kantai, obycode, pavitthrap and gregorycoppola May 19, 2022 17:44

jcnelson added the stacks-2.1 label May 19, 2022

jcnelson mentioned this pull request May 19, 2022

Fix/microblock fees #3144

Merged

jcnelson added 4 commits May 19, 2022 15:49

chore: plumb through the sortition DB handle to determine a matured r…

6861ede

…eward's parent block's evaluated epoch

feat: add unit test coverage for both the old (incorrect) microblock …

18c6200

…fee behavior, the new fee behavior (by way of the new get-block-info? fields), and the transition gating

fix: don't re-insert the same matured reward if we happen to do so fr…

71943e7

…om two or more forks that share the same ancestor block whose reward is about to mature

Merge branch 'feat/btc-stx-exchange-rate' into fix/microblock-fees-3140

8463b20

jcnelson added 2 commits May 20, 2022 15:38

Merge branch 'next' into feat/btc-stx-exchange-rate

a6df76e

Merge branch 'feat/btc-stx-exchange-rate' into fix/microblock-fees-3140

02f7401

gregorycoppola reviewed May 20, 2022

View reviewed changes

jcnelson added 4 commits May 23, 2022 10:30

chore: add unit test for the case where there are no microblocks, to …

e625081

…confirm that we have 0 tx fees from them stored in the MinerReward structs

chore: fix comments

caec40e

Merge branch 'feat/btc-stx-exchange-rate' of https://github.com/stack…

9186b64

…s-network/stacks-blockchain into feat/btc-stx-exchange-rate

Merge branch 'feat/btc-stx-exchange-rate' into fix/microblock-fees-3140

640f62e

pavitthrap reviewed May 24, 2022

View reviewed changes

jcnelson requested review from gregorycoppola and pavitthrap June 1, 2022 16:57

jcnelson added 4 commits June 1, 2022 12:57

fix: run new unit tests, not the old ones

e3304dc

chore: API sync -- coupling miner reward info to matured miner rewards

5432eb5

chore: couple matured miner rewards to miner reward info to avoid a g…

34a9d60

…ratuitous .expect()

fix: user-burn-supports don't get a coinbase reward for poison-microb…

6754379

…lock fee reporting (not a consensus bug since user burn supports are disabled)

kantai reviewed Jun 1, 2022

View reviewed changes

jcnelson added 2 commits June 6, 2022 13:49

fix: don't clobber another test's disk state

b1db0a0

fix: use .to_string() instead of format!()

8543ae5

jcnelson added 2 commits June 6, 2022 14:03

fix: rename inner_insert_matured_miner_payment to inner_insert_mature…

544e88c

…d_miner_reward

chore: cargo fmt

8ed7199

kantai approved these changes Jun 6, 2022

View reviewed changes

jcnelson added 2 commits June 7, 2022 11:14

Merge branch 'next' into feat/btc-stx-exchange-rate

114f64f

Merge branch 'feat/btc-stx-exchange-rate' into fix/microblock-fees-3140

b33ad10

gregorycoppola approved these changes Jun 7, 2022

View reviewed changes

Merge pull request #3144 from stacks-network/fix/microblock-fees-3140

00ca64b

Fix/microblock fees

jcnelson merged commit 875eb49 into next Jun 8, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feat/btc stx exchange rate #3142

Feat/btc stx exchange rate #3142

jcnelson commented May 19, 2022

jcnelson commented May 19, 2022

codecov bot commented May 19, 2022 •

edited

Loading

gregorycoppola left a comment

gregorycoppola May 20, 2022

jcnelson May 22, 2022

pavitthrap May 24, 2022

kantai Jun 1, 2022 •

edited

Loading

jcnelson Jun 6, 2022

gregorycoppola May 20, 2022

jcnelson May 22, 2022 •

edited

Loading

pavitthrap May 24, 2022

jcnelson Jun 1, 2022

pavitthrap May 24, 2022

jcnelson Jun 1, 2022

pavitthrap May 24, 2022

jcnelson May 27, 2022

pavitthrap May 24, 2022

pavitthrap May 24, 2022

jcnelson Jun 1, 2022

kantai Jun 1, 2022

jcnelson Jun 2, 2022

kantai Jun 1, 2022

jcnelson Jun 6, 2022

kantai Jun 1, 2022

jcnelson Jun 6, 2022

kantai Jun 6, 2022

kantai left a comment

gregorycoppola commented Jun 1, 2022

jcnelson commented Jun 6, 2022

kantai left a comment

gregorycoppola left a comment

		mature_miner_payouts: Option<(MinerReward, Vec<MinerReward>, MinerReward)>, // (miner, [users], parent)
		mature_rewards_info: Option<MinerRewardInfo>,

Feat/btc stx exchange rate #3142

Feat/btc stx exchange rate #3142

Conversation

jcnelson commented May 19, 2022

jcnelson commented May 19, 2022

codecov bot commented May 19, 2022 • edited Loading

Codecov Report

gregorycoppola left a 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 Jun 1, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcnelson May 22, 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

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

gregorycoppola commented Jun 1, 2022

jcnelson commented Jun 6, 2022

kantai left a comment

Choose a reason for hiding this comment

gregorycoppola left a comment

Choose a reason for hiding this comment

codecov bot commented May 19, 2022 •

edited

Loading

kantai Jun 1, 2022 •

edited

Loading

jcnelson May 22, 2022 •

edited

Loading