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.

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

chore!: change hash to use consensus encoding #3820

Merged
merged 13 commits into from Mar 16, 2022

Conversation

Cifko
Copy link
Contributor

@Cifko Cifko commented Feb 10, 2022

Description

change hashes to use consensus encoding

Copy link
Contributor

@hansieodendaal hansieodendaal left a comment

Choose a reason for hiding this comment

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

Hi, just a comment of encoding to u64 vs. a single byte for an enum.
Also, this might be a breaking change. Edit: Confirmed, the base node is banning all peers, so the title needs to change.

Looking good otherwise.

base_layer/core/src/proof_of_work/proof_of_work.rs Outdated Show resolved Hide resolved
base_layer/core/src/proof_of_work/proof_of_work.rs Outdated Show resolved Hide resolved
Cifko and others added 3 commits February 10, 2022 14:04
Co-authored-by: Hansie Odendaal <39146854+hansieodendaal@users.noreply.github.com>
Co-authored-by: Hansie Odendaal <39146854+hansieodendaal@users.noreply.github.com>
@Cifko Cifko changed the title chore: change hash to use consensus encoding chore!: change hash to use consensus encoding Feb 10, 2022
@delta1
Copy link
Contributor

delta1 commented Feb 10, 2022

If this breaks sync on the current chain then what purpose does it serve?

@SWvheerden
Copy link
Collaborator

We are going to need it if we want to implement any hashing in another language, but for now immediately, it's not required.

@delta1
Copy link
Contributor

delta1 commented Feb 11, 2022

Yeah if it requires a chain reset it can't go in now.

@delta1 delta1 added the P-do_not_merge Process - Not ready for merging label Feb 11, 2022
hansieodendaal
hansieodendaal previously approved these changes Feb 11, 2022
Copy link
Contributor

@hansieodendaal hansieodendaal left a comment

Choose a reason for hiding this comment

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

LGTM

@hansieodendaal hansieodendaal added the W-consensus_breaking Warn - A change requiring a hard fork to be activated label Feb 11, 2022
Copy link
Member

@sdbondi sdbondi left a comment

Choose a reason for hiding this comment

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

Looks good, just the superfluous length byte for the hash

base_layer/core/src/blocks/block_header.rs Outdated Show resolved Hide resolved
@sdbondi
Copy link
Member

sdbondi commented Mar 9, 2022

In order for the new hashing to work with the current dibbler chain, we should only use the consensus hashing for block version > 2

* development: (54 commits)
  fix(block-sync): use avg latency to determine slow sync peer for block sync (tari-project#3912)
  fix: fix merge mining proxy pool mining (tari-project#3814)
  revert: remove use of blocking tasks for DHT db (reverts tari-project#3887) (tari-project#3901)
  chore: add license info missing from some crates (tari-project#3892)
  fix(core): correctly filter pruned sync peers for block sync (tari-project#3902)
  ci: revert bors squash merge (tari-project#3900)
  fix: update metadata size calculation to use FixedSet.iter()
  docs(rfc): deep links structure convention - deep links is use (tari-project#3897)
  ci: use squash merge for bors (tari-project#3896)
  change struct
  fmt
  chore: fix npm dependencies
  feat: update FFI client user agent string
  fix: update wallet logging config
  fix completed tx index
  ignore clippy
  code review
  fmt
  fix: add bound for number of console_wallet notifications
  remove TODOs
  ...
@sdbondi
Copy link
Member

sdbondi commented Mar 14, 2022

Updated code to continue support for v2 blocks and start using (block template) v3 blocks from height 19000 23000

Manually tested syncing from current dibbler

* development:
  chore: clean up providing seed words from LibWallet (tari-project#3906)
  chore: move tari_script into its own crate (tari-project#3909)
  fix(consensus): check blockchain version within valid range (tari-project#3916)
  ci: fix missing npm deps and add javascript ci (tari-project#3910)
  refactor: use clap as a commands parser (tari-project#3867)
  chore: use git tagged tari_utilities and tari-crypto deps (tari-project#3913)
  fix: aligned tables left (tari-project#3899)
  ci: fix vue build
  v0.29.0
  feat!: add recovery byte to output features (tari-project#3727)
  add ffi ci check (tari-project#3915)
@stringhandler stringhandler merged commit 3a2da1d into tari-project:development Mar 16, 2022
@Cifko Cifko deleted the consensus-encoding branch June 23, 2022 20:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P-do_not_merge Process - Not ready for merging W-consensus_breaking Warn - A change requiring a hard fork to be activated
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants