URL fixes, documentation improvements

[slowriot]

Description

Many URLs in documentation and comments have been broken by a mass search-replace of the word bitcoin with the word BGL. This PR does the following:

• Manually fixes the majority of those links, especially with regard to links that should point to this repository.
• Reinstates valid links to old bitcoin repo pull requests, issues, comments etc.
• Updates links to BGLcore.org to point to bitgesell.ca (this is currently taken care of by a redirect, but it makes sense to use the primary address directly). However, note that many of these are still broken as they originally point to functionality on bitcoincore.org that has not been replicated on bitgesell.ca.
• Reinstates the original content of pre-BGL-fork release notes for older bitcoin versions; the search-replace turned many of these into nonsense, as well as breaking all their links. However, the address for the issue tracker in each release note has been updated to point to the up-to-date issue tracker at this repo.

BTC/BGL PR reward address

ETH/USDT: 0x50b92AB67A3D3DE8c3506D9287893D9a52655486

[van-orton]

Hi @slowriot ! There's some good stuff done (saw other PRs as well), thanks. Please give me a few days to review it.

[slowriot]

Hi @slowriot ! There's some good stuff done (saw other PRs as well), thanks. Please give me a few days to review it.

Thanks. I realise they're big changes that touch a lot of files, so please take your time - there's no hurry on my side. I'll aim to start making more improvements shortly - I'm focusing on getting more of the test suite to pass first, once that's done it'll be a lot easier for approvers to verify that a change hasn't broken anything.

[janus]

Hi @slowriot ! There's some good stuff done (saw other PRs as well), thanks. Please give me a few days to review it.

Thanks. I realise they're big changes that touch a lot of files, so please take your time - there's no hurry on my side. I'll aim to start making more improvements shortly - I'm focusing on getting more of the test suite to pass first, once that's done it'll be a lot easier for approvers to verify that a change hasn't broken anything.

Would you mind concentrate on miner_tests.cpp unit test?

[slowriot]

Hi @slowriot ! There's some good stuff done (saw other PRs as well), thanks. Please give me a few days to review it.

Thanks. I realise they're big changes that touch a lot of files, so please take your time - there's no hurry on my side. I'll aim to start making more improvements shortly - I'm focusing on getting more of the test suite to pass first, once that's done it'll be a lot easier for approvers to verify that a change hasn't broken anything.

Would you mind concentrate on miner_tests.cpp unit test?

Okay, I've had a look at the miner test, and the total failures are interesting - the new created block fails validation so the tests don't even get to start.

Digging into the history of the repo, I suspect some of this issue goes back to the very first commit. Unfortunately there is no detailed commit history between the initial fork from bitcoin, but it looks like bitgesell was forked from somewhere near 0cda5573405d75d695aba417e8f22f1301ded001 of the bitcoin repo. The commit marked as "Initial publish of bitgesell" actually contains a number of differences from this fork point which are worth looking at.

Comparing bitcoin's chainparams.cpp with chainparams.cpp in bitgesell's initial commit, I see a lot of changes that don't make a great deal of sense to me:

Here is a full online diff of chainparams.cpp: https://editor.mergely.com/EcqOlxpk/

Is anyone able to walk through and explain the rationale for each of the original changes in this diff? Without understanding the exact intention behind them, it is hard for me to see which constitute intended behaviour (i.e. the tests should be changed to accomodate them), and which are actually just introducing unintended bugs (i.e. they should be reverted to fix the tests). For example, zeroing consensus.defaultAssumeValid and various other consensus parameters, changing base58Prefixes and the pchMessageStart message start string - what's the rationale for these changes and for the values chosen?

Looking forward to clarification on this.

[janus]

Hi @slowriot ! There's some good stuff done (saw other PRs as well), thanks. Please give me a few days to review it.

Thanks. I realise they're big changes that touch a lot of files, so please take your time - there's no hurry on my side. I'll aim to start making more improvements shortly - I'm focusing on getting more of the test suite to pass first, once that's done it'll be a lot easier for approvers to verify that a change hasn't broken anything.

Would you mind concentrate on miner_tests.cpp unit test?

Okay, I've had a look at the miner test, and the total failures are interesting - the new created block fails validation so the tests don't even get to start.

Digging into the history of the repo, I suspect some of this issue goes back to the very first commit. Unfortunately there is no detailed commit history between the initial fork from bitcoin, but it looks like bitgesell was forked from somewhere near 0cda5573405d75d695aba417e8f22f1301ded001 of the bitcoin repo. The commit marked as "Initial publish of bitgesell" actually contains a number of differences from this fork point which are worth looking at.

Comparing bitcoin's chainparams.cpp with chainparams.cpp in bitgesell's initial commit, I see a lot of changes that don't make a great deal of sense to me:

Here is a full online diff of chainparams.cpp: https://editor.mergely.com/EcqOlxpk/

Is anyone able to walk through and explain the rationale for each of the original changes in this diff? Without understanding the exact intention behind them, it is hard for me to see which constitute intended behaviour (i.e. the tests should be changed to accomodate them), and which are actually just introducing unintended bugs (i.e. they should be reverted to fix the tests). For example, zeroing consensus.defaultAssumeValid and various other consensus parameters, changing base58Prefixes and the pchMessageStart message start string - what's the rationale for these changes and for the values chosen?

Looking forward to clarification on this.

Give me some time, I would get back to you.

[janus]

For example, zeroing consensus.defaultAssumeValid and various other consensus parameters, changing base58Prefixes and the pchMessageStart message start string - what's the rationale for these changes and for the values chosen?

The base58Prefixes are needed to differentiate BGL addresses from BTC addresses. Without this you would have same addresses. BGL uses same code as BTC but different base58Prefixes.

For pchMessageStart:
Those are the magic bytes. They prepend every single network message so that the message can be identified as being a BGL message.
They're arbitrary. The common recommendation for new altcoins is to just randomly generate new magic bytes.

The below are not needed because BGL has by default inherited those features:

consensus.BIP16Exception = uint256S("0x00000000000002dc756eebf4f49723ed8d30cc28a5f108eb94b1ba88ac4f9c22");
        consensus.BIP34Height = 227931;
        consensus.BIP34Hash = uint256S("0x000000000000024b89b42a942fe0d9fea3bb44ab7bd1b19115dd6a759c0808b8");
        consensus.BIP65Height = 388381; // 000000000000000004c2b624ed5d7756c508d90fd0da2c7c679febfa6c4735f0
        consensus.BIP66Height = 363725; // 00000000000000000379eaa19dce8c9b722d46ae6a57c2f1a988119488b50931
        consensus.CSVHeight = 419328; // 000000000000000004a1b34462cb8aeebd5799177f7a29cf28f2d1961716b5b5
        consensus.SegwitHeight = 481824; // 0000000000000000001c8018d9cb3b742ef25114f27563e3fc4a1902167f9893

The main issue out there is Genesis nBits that was not based on powLimit. But I strongly believe that there should be a way out to clean up miner_tests.cpp and get it to pass.

consensus.nMinimumChainWork and consensus.defaultAssumeValid change at update. And forking Bitcoin code is like update. The value of nMinimumChainWork is the lowest at Genesis(difficulty 1). defaultAssumeValid is used to confirm the best chain ancestors.

If you have further questions, don't hesitate to ask.

For more information, please check out references the below:
https://medium.com/@jordan.baczuk/how-to-fork-bitcoin-c39139506443
https://bitcoin.stackexchange.com/questions/26869/what-is-chainwork
https://bitcointalk.org/index.php?topic=136628.0
https://bitcoin.stackexchange.com/questions/26869/what-is-chainwork
https://github.com/CryptAxe/info/blob/master/AssumeValid.md

https://medium.com/@jordan.baczuk/how-to-fork-bitcoin-part-2-59b9eddb49a4
https://bitcointalk.org/index.php?topic=2046543.0

[slowriot]

For pchMessageStart:
Those are the magic bytes. They prepend every single network message so that the message can be identified as being a BGL message.
They're arbitrary. The common recommendation for new altcoins is to just randomly generate new magic bytes.

Thanks for the fast and detailed reply. Having now had time to look at this in more depth, I can see a number of problems mostly related to these changes (for example, the prefixes, and the pubkey) that have been related to a number of tests which have been failing so far.

Unfortunately, it looks like some developer attempted to work around the test failures by messing with the base58 implementation (which I addressed in PR #53) - fixing this actually breaks some other tests (several in key_tests), and I now understand why. The base58 implementation itself had been poorly modified to allow prefix-10 addresses to decode as if they were prefix-0 addresses - a terrible idea - which almost worked (but not quite), and had many knock-on effects elsewhere. What's worse, correct tests were modified, which would previously have failed, so that they pass despite incorrect output from this implementation. I'm about to raise another PR to fix some of these issues, but all of this work will be predicated on PR #53, which is needed to get a correctly working base58 implementation again - many things will be broken across the codebase in small ways until that is merged.

Looking at the miner test, it fails to accept the genesis block, on the grounds that proof of work failed. As you say, the fact that nbits did not take the POW limit into account, means that it's likely the genesis block's hash is greater than the POW bits, meaning it cannot logically have been mined with the current POW limit. This is a great example of why it's a bad idea to release to the public while ignoring failing tests ;) But I agree with you that it should be possible to work around this in the tests and elsewhere, so I'll aim to do that.

I'll pick off some of these issues over the coming days and work my way towards the miner test in the process.

[janus]

For pchMessageStart:
Those are the magic bytes. They prepend every single network message so that the message can be identified as being a BGL message.
They're arbitrary. The common recommendation for new altcoins is to just randomly generate new magic bytes.

Thanks for the fast and detailed reply. Having now had time to look at this in more depth, I can see a number of problems mostly related to these changes (for example, the prefixes, and the pubkey) that have been related to a number of tests which have been failing so far.

Unfortunately, it looks like some developer attempted to work around the test failures by messing with the base58 implementation (which I addressed in PR #53) - fixing this actually breaks some other tests (several in key_tests), and I now understand why. The base58 implementation itself had been poorly modified to allow prefix-10 addresses to decode as if they were prefix-0 addresses - a terrible idea - which almost worked (but not quite), and had many knock-on effects elsewhere. What's worse, correct tests were modified, which would previously have failed, so that they pass despite incorrect output from this implementation. I'm about to raise another PR to fix some of these issues, but all of this work will be predicated on PR #53, which is needed to get a correctly working base58 implementation again - many things will be broken across the codebase in small ways until that is merged.

Looking at the miner test, it fails to accept the genesis block, on the grounds that proof of work failed. As you say, the fact that nbits did not take the POW limit into account, means that it's likely the genesis block's hash is greater than the POW bits, meaning it cannot logically have been mined with the current POW limit. This is a great example of why it's a bad idea to release to the public while ignoring failing tests ;) But I agree with you that it should be possible to work around this in the tests and elsewhere, so I'll aim to do that.

I'll pick off some of these issues over the coming days and work my way towards the miner test in the process.

@slowriot please check out janus branch. Virtually all the major unit tests failures are already resolved or partially resolved except miner_tests and transaction_tests. Concentrate on these. I would add more to the list later today.

Don't hesitate to ask questions.

[janus]

For pchMessageStart:
Those are the magic bytes. They prepend every single network message so that the message can be identified as being a BGL message.
They're arbitrary. The common recommendation for new altcoins is to just randomly generate new magic bytes.

Thanks for the fast and detailed reply. Having now had time to look at this in more depth, I can see a number of problems mostly related to these changes (for example, the prefixes, and the pubkey) that have been related to a number of tests which have been failing so far.

Unfortunately, it looks like some developer attempted to work around the test failures by messing with the base58 implementation (which I addressed in PR #53) - fixing this actually breaks some other tests (several in key_tests), and I now understand why. The base58 implementation itself had been poorly modified to allow prefix-10 addresses to decode as if they were prefix-0 addresses - a terrible idea - which almost worked (but not quite), and had many knock-on effects elsewhere. What's worse, correct tests were modified, which would previously have failed, so that they pass despite incorrect output from this implementation. I'm about to raise another PR to fix some of these issues, but all of this work will be predicated on PR #53, which is needed to get a correctly working base58 implementation again - many things will be broken across the codebase in small ways until that is merged.

Looking at the miner test, it fails to accept the genesis block, on the grounds that proof of work failed. As you say, the fact that nbits did not take the POW limit into account, means that it's likely the genesis block's hash is greater than the POW bits, meaning it cannot logically have been mined with the current POW limit. This is a great example of why it's a bad idea to release to the public while ignoring failing tests ;) But I agree with you that it should be possible to work around this in the tests and elsewhere, so I'll aim to do that.

I'll pick off some of these issues over the coming days and work my way towards the miner test in the process.

Pending unit tests(ones still failing)

1. addrman_tests (low hanging apple)
2. bloom_tests(low hanging apple, you may also leave this one)
3. merklebock_tests(don't worry yourself, leave it for now)
4. miner_tests(serious)
5. transaction_tests
6. versionbits_tests

[wu-emma]

Hi @slowriot Thanks for the contribution, tx is:
https://etherscan.io/tx/0x8467a9f7f8091893ead4a237076015895039e9991b504b956209010781333922
guys would check other PRs and would be tracking it.

[janus]

Hi @slowriot Thanks for the contribution, tx is:
https://etherscan.io/tx/0x8467a9f7f8091893ead4a237076015895039e9991b504b956209010781333922
guys would check other PRs and would be tracking it.

For pchMessageStart:
Those are the magic bytes. They prepend every single network message so that the message can be identified as being a BGL message.
They're arbitrary. The common recommendation for new altcoins is to just randomly generate new magic bytes.

Thanks for the fast and detailed reply. Having now had time to look at this in more depth, I can see a number of problems mostly related to these changes (for example, the prefixes, and the pubkey) that have been related to a number of tests which have been failing so far.
Unfortunately, it looks like some developer attempted to work around the test failures by messing with the base58 implementation (which I addressed in PR #53) - fixing this actually breaks some other tests (several in key_tests), and I now understand why. The base58 implementation itself had been poorly modified to allow prefix-10 addresses to decode as if they were prefix-0 addresses - a terrible idea - which almost worked (but not quite), and had many knock-on effects elsewhere. What's worse, correct tests were modified, which would previously have failed, so that they pass despite incorrect output from this implementation. I'm about to raise another PR to fix some of these issues, but all of this work will be predicated on PR #53, which is needed to get a correctly working base58 implementation again - many things will be broken across the codebase in small ways until that is merged.
Looking at the miner test, it fails to accept the genesis block, on the grounds that proof of work failed. As you say, the fact that nbits did not take the POW limit into account, means that it's likely the genesis block's hash is greater than the POW bits, meaning it cannot logically have been mined with the current POW limit. This is a great example of why it's a bad idea to release to the public while ignoring failing tests ;) But I agree with you that it should be possible to work around this in the tests and elsewhere, so I'll aim to do that.
I'll pick off some of these issues over the coming days and work my way towards the miner test in the process.

Pending unit tests(ones still failing)

1. addrman_tests (low hanging apple)

2. bloom_tests(low hanging apple, you may also leave this one)

3. merklebock_tests(don't worry yourself, leave it for now)

4. miner_tests(serious)

5. transaction_tests

6. versionbits_tests

@slowriot
I have started working on miner_tests and I am making some progress.

[slowriot]

I'll pick off some of these issues over the coming days and work my way towards the miner test in the process.

Pending unit tests(ones still failing)

1. addrman_tests (low hanging apple)

2. bloom_tests(low hanging apple, you may also leave this one)

3. merklebock_tests(don't worry yourself, leave it for now)

4. miner_tests(serious)

5. transaction_tests

6. versionbits_tests

@slowriot
I have started working on miner_tests and I am making some progress.

Noted, thanks. I hadn't realised development was more advanced on a branch other than master.

I would suggest merging #53 to master in any case, as any development work by other contributors done on master will inevitably be broken until that is resolved - although I recognise now that the same solution has also been independently implemented on your branch.

I will target any further PRs against the janus release branch.

[janus]

I'll pick off some of these issues over the coming days and work my way towards the miner test in the process.

Pending unit tests(ones still failing)

1. addrman_tests (low hanging apple)

2. bloom_tests(low hanging apple, you may also leave this one)

3. merklebock_tests(don't worry yourself, leave it for now)

4. miner_tests(serious)

5. transaction_tests

6. versionbits_tests

@slowriot
I have started working on miner_tests and I am making some progress.

Noted, thanks. I hadn't realised development was more advanced on a branch other than master.

I would suggest merging #53 to master in any case, as any development work by other contributors done on master will inevitably be broken until that is resolved - although I recognise now that the same solution has also been independently implemented on your branch.

I will target any further PRs against the janus release branch.
@slowriot
Let me know from the pending list the one you are working on.

[slowriot]

Let me know from the pending list the one you are working on.

If you're looking at miner_tests, I'll start to look at transaction_tests later today, as that is very important in principle. However, it reports 6705 failures (!) which may take a little while to get through... I will also have a look at versionbits_tests and addrman_tests in the interim, which may be quick to fix.

Additionally to your list, I note that script_tests and util_tests are also failing on the janus branch - just FYI.

[janus]

Let me know from the pending list the one you are working on.

If you're looking at miner_tests, I'll start to look at transaction_tests later today, as that is very important in principle. However, it reports 6705 failures (!) which may take a little while to get through... I will also have a look at versionbits_tests and addrman_tests in the interim, which may be quick to fix.

Additionally to your list, I note that script_tests and util_tests are also failing on the janus branch - just FYI.

script_tests and util_tests are passing. I have pushed my local version of script_tests, and I would check what I missed out while pushing util_tests.
Just work on any of these, versionbits_tests and addrman_tests, for now.

[slowriot]

Just work on any of these, versionbits_tests and addrman_tests, for now.

• addrman_tests is fixed with addrman fixes #59
• I'll look at versionbits_tests next.

[janus]

Please stick to Keccak wherever it is used. We are not looking for quick-fix.

[slowriot]

Please stick to Keccak wherever it is used. We are not looking for quick-fix.

As explained in detail on the other thread at #59 (comment):

I understand that there's a general policy to use keccak for hashing; however, the algorithms used in addrman are not for generating public-facing hashes of addresses; the change I made applies specifically to the logic selecting buckets for addresses, which must be performed in a predictable way. The implementation in bitcoin core works well for this, and the scattergun change that was made to this was of zero benefit, and some detriment.

Please note that the changes here do not alter the use of keccak in any role associated with address or other data hashing in the blockchain. It is purely an internal implementation detail of addrman, that should never have been allowed to be broken to begin with.

You say "we are not looking for a quick fix" which is precisely why I made the change in this way - the design of addrman in bitcoin is based around probabilistic bucket distributions which are derived from the nature of the algorithm used; if the algorithm is changed arbitrarily, you lose that probabilistic distribution, and buckets can become skewed. This fixes the algorithm, rather than adjusting the tests to match a broken algorithm.

A "quick fix" solution here would instead accept the inferior algorithm, and change the tests so that they pass regardless. If that is actually what you want, please let me know, and I will provide an alternative pull request - but please note that forcing your non-conformant algorithm derived vaguely from a keccak base here will only result in an insecure addrman implementation that is vulnerable to denial of service attacks due to bucket imbalance.

[janus]

Please stick to Keccak wherever it is used. We are not looking for quick-fix.

As explained in detail on the other thread at #59 (comment):

I understand that there's a general policy to use keccak for hashing; however, the algorithms used in addrman are not for generating public-facing hashes of addresses; the change I made applies specifically to the logic selecting buckets for addresses, which must be performed in a predictable way. The implementation in bitcoin core works well for this, and the scattergun change that was made to this was of zero benefit, and some detriment.
Please note that the changes here do not alter the use of keccak in any role associated with address or other data hashing in the blockchain. It is purely an internal implementation detail of addrman, that should never have been allowed to be broken to begin with.

You say "we are not looking for a quick fix" which is precisely why I made the change in this way - the design of addrman in bitcoin is based around probabilistic bucket distributions which are derived from the nature of the algorithm used; if the algorithm is changed arbitrarily, you lose that probabilistic distribution, and buckets can become skewed. This fixes the algorithm, rather than adjusting the tests to match a broken algorithm.

A "quick fix" solution here would instead accept the inferior algorithm, and change the tests so that they pass regardless. If that is actually what you want, please let me know, and I will provide an alternative pull request - but please note that forcing your non-conformant algorithm derived vaguely from a keccak base here will only result in an insecure addrman implementation that is vulnerable to denial of service attacks due to bucket imbalance.

Let's go with Keccak.

[slowriot]

Please stick to Keccak wherever it is used. We are not looking for quick-fix.

You say "we are not looking for a quick fix" which is precisely why I made the change in this way ...
... but please note that forcing your non-conformant algorithm derived vaguely from a keccak base here will only result in an insecure addrman implementation that is vulnerable to denial of service attacks due to bucket imbalance.

Let's go with Keccak.

Before I begin work on this, can I please confirm that you are a maintainer of this project who is authorised to make such a decision, and that the project stakeholders understand that you are asking me to adjust failing tests to pass for a faulty algorithm, rather than implementing a valid algorithm for valid tests? Are all parties aware of the security implications of altering the addrman cheaphash algorithm and the loss of assurances about balance of buckets that it entails? If so, I'll proceed for the bounty, but my professional advice is not to take this route.