Update for get-block-info in SIP-021

[friedger]

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?

[wileyj]

looks good to me, i don't see any logical issues with what's proposed as addendum here

[friedger]

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

[wileyj]

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]

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]

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]

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]

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]

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?:

• burnchain-header-hash
• id-header-hash
• header-hash
• miner-address
• time
• vrf-seed
• block-reward
• miner-spend-total
• miner-spend-winner

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]

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]

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

[obycode]

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]

Returning burnchain time sounds correct

[jcnelson]

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

[obycode]

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]

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]

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

[314159265359879]

Valuable minor clarifications. LGTM!

[AcrossfireX]

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.

[whoabuddy]

I missed this when approving/merging SIP-021 in #155.

Should we point the change here to the main branch now to include the changes?

Should this be revised to the end of the document per @jiga's comment?
https://github.com/stacksgov/sips/pull/178/files#r1662687245

Anything else that needs to be done to close this out?

[wileyj]

Should we point the change here to the main branch now to include the changes?

that probably makes the most sense and can be done without any change from PR submitter.

i think the location is preference only, but there is precedence for adding it after the abstract section: https://github.com/stacksgov/sips/blob/main/sips/sip-022/sip-022-emergency-pox-fix.md?plain=1

The base branch was changed.

[obycode]

I changed the target branch. I do think it makes more sense to put this addendum at the end. Putting it at the beginning seems more appropriate for something that is more significant and deserves more attention.

[wileyj]

I changed the target branch. I do think it makes more sense to put this addendum at the end. Putting it at the beginning seems more appropriate for something that is more significant and deserves more attention.

@friedger is that a change you think you can make here?