Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Remove BlockBase and ConsensusData #2296

Merged
merged 3 commits into from
Feb 5, 2021
Merged

Remove BlockBase and ConsensusData #2296

merged 3 commits into from
Feb 5, 2021

Conversation

erikzhang
Copy link
Member

No description provided.

erikzhang and others added 2 commits February 5, 2021 16:44
@erikzhang @shargon
Update src/neo/Network/P2P/Payloads/Header.cs
fc44f64 
Co-authored-by: Shargon <shargon@gmail.com>
@erikzhang
Fix UT
9a5af66
roman-khimov
roman-khimov reviewed Feb 5, 2021
View reviewed changes
src/neo/Network/P2P/Payloads/Block.cs
public UInt256 MerkleRoot => Header.MerkleRoot;
public ulong Timestamp => Header.Timestamp;
public uint Index => Header.Index;
public byte PrimaryIndex => Header.PrimaryIndex;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This simplifies things a lot, but don't we also want to have a nonce in the header? It might be useful to have for contracts.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The nonce in the block is an unreliable random number, and contracts should not rely on it.

src/neo/Network/P2P/Payloads/Header.cs
sizeof(uint) + // Index
sizeof(byte) + // PrimaryIndex
UInt160.Length + // NextConsensus
1 + Witness.Size; // Witness
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can even keep the same header size by removing this 1.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have already discussed this before. 😆

@erikzhang erikzhang merged commit b690466 into master Feb 5, 2021
@erikzhang erikzhang deleted the remove-blockbase-consensusdata branch February 5, 2021 10:28
Ashuaidehao added a commit to Ashuaidehao/neo-devpack-dotnet that referenced this pull request Feb 7, 2021
@Ashuaidehao
sync neo-project/neo#2296
f1c0cd7
@Ashuaidehao Ashuaidehao mentioned this pull request Feb 7, 2021
Merged
shargon pushed a commit to neo-project/neo-devpack-dotnet that referenced this pull request Feb 7, 2021
@Ashuaidehao
sync remove consensus data (#462)
2f456a8 
* sync neo-project/neo#2296

* upgrade neo
@roman-khimov roman-khimov mentioned this pull request Feb 15, 2021
Closed
ixje added a commit to CityOfZion/neo-mamba that referenced this pull request Apr 8, 2021
@ixje
https://github.com/neo-project/neo/pull/2296
2505b92
ixje added a commit to CityOfZion/neo-mamba that referenced this pull request Apr 30, 2021
@ixje
Rc1 (#84)
8b18e30 
* neo-project/neo#2295
* neo-project/neo#2290
* neo-project/neo#2292
* neo-project/neo#2296
* neo-project/neo#2301
* neo-project/neo#2298
* neo-project/neo#2312
* neo-project/neo#2300
* neo-project/neo#2333
* neo-project/neo#2337
* neo-project/neo#2331
* neo-project/neo#2332
* neo-project/neo#2343
* neo-project/neo#2339
* neo-project/neo#2350
* neo-project/neo#2351
* neo-project/neo#2353
* neo-project/neo#2356
* neo-project/neo#2375
* neo-project/neo#2377
* neo-project/neo#2379
* https://github.com/neo-project/neo/pull/2392/files
* neo-project/neo#2400
* audit updates
* refactor and bump VM requirement
@Jim8y Jim8y mentioned this pull request Apr 30, 2021
Closed
roman-khimov added a commit to nspcc-dev/neo-go that referenced this pull request Jul 27, 2024
@roman-khimov
block: add PrimaryIndex to the stack item of block
1a48f1c 
This is a bad one.

$ ./bin/neo-go contract testinvokefunction -r https://rpc10.n3.nspcc.ru:10331 0xda65b600f7124ce6c79950c1772a36403104f2be getBlock 5762000
{
  "state": "HALT",
  "gasconsumed": "202812",
  "script": "AtDrVwARwB8MCGdldEJsb2NrDBS+8gQxQDYqd8FQmcfmTBL3ALZl2kFifVtS",
  "stack": [
    {
      "type": "Array",
      "value": [
        {
          "type": "ByteString",
          "value": "vq5IPTPEDRhz0JA4cQKIa6/o97pnJt/HfVkDRknd1rg="
        },
        {
          "type": "Integer",
          "value": "0"
        },
        {
          "type": "ByteString",
          "value": "zFYF3LGaTKdbqVX99shaBUzTq9YjXb0jaPMjk2jdSP4="
        },
        {
          "type": "ByteString",
          "value": "AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA="
        },
        {
          "type": "Integer",
          "value": "1722060076994"
        },
        {
          "type": "Integer",
          "value": "5293295626238767595"
        },
        {
          "type": "Integer",
          "value": "5762000"
        },
        {
          "type": "ByteString",
          "value": "LIt05Fpxhl/kXMX3EAGIASyOSQs="
        },
        {
          "type": "Integer",
          "value": "0"
        }
      ]
    }
  ],
  "exception": null,
  "notifications": []
}

$ ./bin/neo-go contract testinvokefunction -r http://seed3.neo.org:10332 0xda65b600f7124ce6c79950c1772a36403104f2be getBlock 5762000
{
  "state": "HALT",
  "gasconsumed": "202812",
  "script": "AtDrVwARwB8MCGdldEJsb2NrDBS+8gQxQDYqd8FQmcfmTBL3ALZl2kFifVtS",
  "stack": [
    {
      "type": "Array",
      "value": [
        {
          "type": "ByteString",
          "value": "vq5IPTPEDRhz0JA4cQKIa6/o97pnJt/HfVkDRknd1rg="
        },
        {
          "type": "Integer",
          "value": "0"
        },
        {
          "type": "ByteString",
          "value": "zFYF3LGaTKdbqVX99shaBUzTq9YjXb0jaPMjk2jdSP4="
        },
        {
          "type": "ByteString",
          "value": "AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA="
        },
        {
          "type": "Integer",
          "value": "1722060076994"
        },
        {
          "type": "Integer",
          "value": "5293295626238767595"
        },
        {
          "type": "Integer",
          "value": "5762000"
        },
        {
          "type": "Integer",
          "value": "6"
        },
        {
          "type": "ByteString",
          "value": "LIt05Fpxhl/kXMX3EAGIASyOSQs="
        },
        {
          "type": "Integer",
          "value": "0"
        }
      ]
    }
  ],
  "exception": null,
  "notifications": []
}

9 fields vs 10, notice the primary index right after the block number.

Back when ac52765 initially added Ledger I've
used neo-project/neo#2215 as a reference and it was
correct (no primary index). But then neo-project/neo#2296
came into the C# codebase and while it looked like a pure refactoring it
actually did add the primary index as well and this wasn't noticed. It wasn't
noticed even when 3a4e0ca had touched some
nearby code. In short, we had a completely wrong implementation of this call
for more than three years. But looks like it's not a very popular one.

Signed-off-by: Roman Khimov <roman@nspcc.ru>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@roman-khimov roman-khimov roman-khimov left review comments

@shargon shargon shargon approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@erikzhang @shargon @roman-khimov