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

Correct hash prefixes used in tests and use consistent pubkey, add tests to reject bitcoin addresses #57

Closed
wants to merge 18 commits into from

Conversation

slowriot
Copy link
Contributor

@slowriot slowriot commented Sep 1, 2021

Description

PLEASE NOTE: This PR depends on, and includes the changes made in, #53. Those should be merged first, which will make the specific changes here more apparent.

  • The address hashes used in some tests are bitcoin format addresses, with an incorrect prefix for bitgesell. Previously tests and even the base58 implementation had been bodged to make these work. This PR converts those BTC-prefix addresses to their BGL equivalent.
  • Some tests had been initialised with the incorrect pubkey (again, the one from BTC's genesis block) - these have been amended for consistency here to match the genesis pubkey of BGL.
  • Certain tests and asserts had been disabled, which have now been re-enabled and are passing.
  • Additional tests to verify addresses are valid before attempting to test decoding and encoding with them
  • Additional tests to verify BTC-format addresses are rejected as they should be

See commit messages for specific details of each change.

Note, the content of this PR was originally targeted against master, but the changes have also been added to a separate PR against the janus release branch shortly, to ensure consistency:

BTC/BGL PR reward address

ETH/USDT: 0x50b92AB67A3D3DE8c3506D9287893D9a52655486

…tcoin core, and turn off warning silencing for implicit fallthrough
…ior test addresses are BTC addresses (prefix 0), and should not be valid
…use the correct pubkey of the BGL genesis block instead
@slowriot slowriot mentioned this pull request Sep 6, 2021
@janus
Copy link
Collaborator

janus commented Sep 7, 2021

This PR won't be merged, it didn't bring anything of great value or something not already solved.

@slowriot
Copy link
Contributor Author

slowriot commented Sep 7, 2021

Why this change?

Your comments do not appear in-line to the code, so it is not clear to me which you are enquiring about. However, I will attempt to explain the change further if needed. The tests are running with Bitcoin addresses, not Bitgessel addresses, and if they pass without this change, that is an error in the programming of the tests (having inherited them from BTC). Those addresses are not valid in BGL, and the tests should reflect this. The tests pass with these correct addresses - which are the same addresses of the bitcoin-derived tests, with BGL address prefixes, which ensure everything else remains equal in the tests, and achieves your stated goal of staying "as close as possible to [the] Bitcoin codebase".

The genesis block initiated in the tests also differs from the genesis block of BGL, which means you are testing against a blockchain with different initialisation parameters from the production codebase - the result is that the tests are potentially meaningless. The changes in this PR ensure correct conformant addresses are used for testing, and the genesis block is initiated with the correct pubkey.

janus pushed a commit that referenced this pull request Nov 11, 2021
a44caf65fe Merge bitcoin-core/univalue-subtree#28: Import fixes for sanitizer reported issues
135254331e Import fixes for sanitizer reported issues
d5fb86940e refactor: use c++11 range based for loop in checkObject
ff9c379304 refactor: Use nullptr (c++11) instead of NULL
08a99754d5 build: use ax_cxx_compile_stdcxx.m4 to check for C++11 support
66d3713ce7 Merge bitcoin-core/univalue-subtree#29: ci: travis -> cirrus
808d487292 ci: travis -> cirrus
c390ac375f Merge bitcoin-core/univalue-subtree#19: Split sources for easier buildsystem integration
4a5b0a1c65 build: Move source entries out to sources.mk
6c7d94b33c build: cleanup wonky gen usage
a222637c6d Merge #23: Merge changes from jgarzik/univalue@1ae6a23
f77d0f718d Merge commit '1ae6a231a0169938eb3972c1d48dd17cba5947e1' into HEAD
1ae6a231a0 Merge pull request #57 from MarcoFalke/test_fix
92bdd11f0b univalue_write: remove unneeded sstream.h include
ffb621c130 Merge pull request #56 from drodil/remove_sstream_header
f33acf9fe8 Merge commit '7890db9~' into HEAD
66e0adec4d Remove unnecessary sstream header from univalue.h
88967f6586 Version 1.0.4
1dc113dbef Merge pull request #50 from luke-jr/pushKV_bool
72392fb227 [tests] test pushKV for boolean values
c23132bcf4 Pushing boolean value to univalue correctly
81faab26a1 Merge pull request #48 from fwolfst/47-UPDATE_MIT_LINK_TO_HTTPS
b17634ef24 Update URLs to MIT license.
88ab64f6b5 Merge pull request #46 from jasonbcox/master
35ed96da31 Merge pull request #44 from MarcoFalke/Mf1709-univalue-cherrypick-explicit
420c226290 Merge pull request #45 from MarcoFalke/Mf1710-univalue-revert-test

git-subtree-dir: src/univalue
git-subtree-split: a44caf65fe55b9dd8ddb08f04c0f70409efd53b3
@madnadyka madnadyka closed this Jul 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants