stacksgov / sips

Community-submitted Stacks Improvement Proposals (SIPs)
130 stars 80 forks source link

Update for get-block-info #178

Open friedger opened 1 month ago

friedger commented 1 month ago

This PR adds an Addendum to SIP 021-Nakamoto v1 that describes the impact of Fast Blocks to the clarity function get-block-info?

The function should be replaced by get-stacks-block-info? and get-tenure-info?

friedger commented 1 month ago

Is id-header-hash still meaningful on the stacks block level as the stack chain does not fork anymore on that level?

wileyj commented 1 month ago

Is id-header-hash still meaningful on the stacks block level as the stack chain does not fork anymore on that level?

very good question - @obycode any insight here?

obycode commented 1 month ago

Since this add-on is for minor changes to Clarity, let's also add removal of index-of and element-at, forcing the use of the variants ending in ?. What do you think?

Never mind. After thinking on this a bit more, I don't think we should include it here.

obycode commented 1 month ago

very good question - @obycode any insight here?

Copying the responses from Slack here: friedger: The block hash is the stacks block hash. The consensus hash from tenure? Aaron: yep -- it's the tenure's consensus hash Aaron: so, even if the tenure spanned multiple bitcoin blocks (due to a tenure-extend), the same consensus hash would be used for the id-header-hash

obycode commented 1 month ago

At today's WG sync, we discussed the value in minimizing changes to the function as get-block-info? instead of removing it and adding get-stacks-block-info?, with the goal of minimizing unnecessary changes for developers. Let's continue that discussion here.

If we left get-block-info? as-is, we would just be modifying the time property to return the new nakamoto time instead of the burn-block time. The rest of the properties could still return the values from the current tenure. If we did that, I would still want to add get-tenure-info? which takes in a tenure height and includes the properties mentioned in this PR.

friedger commented 1 month ago

My concern is that newly onboarded developers (after Nakamoto) get confused about the properties and what they mean. In particular, the change to vrf-seed.

The Nakamoto upgrade should be celebrated as the better solution. We can provide a tool/add-on for clarinet that converts Clarity 2 code to Clarity 3 code.

obycode commented 1 month ago

There was some more discussion about this and there seems to be a preference to not add new functions, but instead just add a new property to get-block-info? for the new timestamp.

Just to have everything in one place for ease of discussion, here are the current properties for get-block-info?:

We have one new property to add, which is a timestamp included in the header of each Nakamoto block.

I can see how some of these properties could be confusing to users if we leave get-block-info? as-is. For example, they may assume that the block-reward, miner-spend-total, and miner-spend-winner are meaningful for each block. Two options to avoid this bad assumption:

  1. only include non-zero values for those properties in the block with the tenure change
  2. move them to a separate, tenure-specific function as suggested in this PR

Thinking through all of this, my current preference is:

  1. Add a stacks-time property to get-block-info? to return the new timestamp
  2. Modify the way block-reward, miner-spend-total, and miner-spend-winner are set in Nakamoto so that they are only non-zero on blocks with a tenure change.

I still have a concern about vrf-seed. Might users think they can expect a unique VRF seed with each block? Should we return a none if it's not a tenure change block? If we return none, then you can't differentiate between a bad block-height and a block that is not a tenure change.

@friedger, @jcnelson, @kantai please add your thoughts.

friedger commented 1 month ago

I still have a concern about vrf-seed. Might users think they can expect a unique VRF seed with each block?

Yes, because that was the case until now (if you ignore microblocks). Can we create something random out of the signers signatures?

I like the solution that the function returns none if not tenure height. To use the function I would need to use tenure-height, or (- tenure-height N) for the Nth previous vrf seed, correct ?

obycode commented 1 month ago

Just had another chat about this, and I think we came full circle back around to your original proposal 😅.

obycode commented 3 weeks ago

Do you think calling (get-stacks-block-info? time u1234) where 1234 is a pre-epoch 3.0 block should return the burnchain time or should it return none? I think my preference is to make it return the burnchain time, but wanted to get input from someone else.

friedger commented 3 weeks ago

Returning burnchain time sounds correct

jcnelson commented 3 weeks ago

Agree with @friedger -- the timestamp of a Stacks block pre-Nakamoto is the burnchain time.

obycode commented 3 weeks ago

I think we may also need to add a new function (get-tenure-for-block? block-height) which returns an (optional uint) which is the tenure height of the specified block if block-height <= tip-height. The other option would be (get-tenure-info-for-block? block-height) which functions like get-tenure-info? except it takes in a block height instead of a tenure height and has an additional property, height, which returns the tenure height at the specified block.

obycode commented 3 weeks ago

Based on discussion from today's call, I will change the implementation of get-tenure-info? to take in a burn block height as its parameter. tenure-height can be a new additional property.

obycode commented 3 weeks ago

Another change here... get-tenure-info? will take a Stacks block height as its parameter. Implemented in stacks-network/stacks-core#4846

AcrossfireX commented 2 weeks ago

Acrossfire (SIP Editor) - signing off for SIP Editors on this change. This has been discussed with the community in relevant community conversations already so the community should be well positioned to understand this.