addrman fixes

[slowriot]

Description

Addrman tests currently fail for a number of reasons. This PR addresses them, repairing the underlying functionality as much as possible, while keeping tests as consistent with Bitcoin Core as possible, for easier maintenance in future.

Please note this PR is targeted against the janus/release-0.1.7 branch, not master.

This PR fixes all addrman issues, enabling the addrman_tests test suite to pass. Run the test suite with src/test/test_BGL --log_level=all --run_test=addrman_tests -- DEBUG_LOG_OUT

Changes:

• Update the addrman test suite to match that in Bitcoin core, ensuring test methodology is consistent and future-proof
• Switch addrman's CheapHash back to SHA256 to provide a consistent backwards-compatible algorithm for bucket selection.
• Switch addrman's SHA256 CheapHash implementation to be consistent with the algorithm used in bitcoin core, so addrman bucket selection behaviour is consistent with expected probabilities and results. This is important, as addrman's stochastic bucket selection depends on a consistently applied hash mechanism, and the arbitrary change to switch to Keccak and then create a new untested hash algorithm (rightly) broke tests, while providing zero benefit.
• Enable addrman to operate in deterministic mode, where the nKey is a fixed magic number; this enables reliable testing. This functionality is functionally equivalent with that found in Bitcoin core (but added after Bitgesell's fork point), but implemented in a clean minimal way.
• Make sure exclusive locks are applied and their presence asserted consistently through the addrman code (in line with locking behaviour in bitcoin core).
• Reinstate consistency check, no longer gated behind a debug define - again, consistent with bitcoin core behaviour.
• Adjust the addrman Good() function to always test before evict; this was the cause of a crucial bug. Accepting a bool as a second parameter meant that in at least one case, an nTime was passed to the function in the bool slot by mistake, which led to entries not being evicted when they should have - this was tested for by the evictionworks test.
• Add new testcases to verify CAddrDB functionality, and ensure they pass.

Test status updated progressively in comments below.

BTC/BGL PR reward address

ETH/USDT: 0x50b92AB67A3D3DE8c3506D9287893D9a52655486

[slowriot]

Initial test status:

[slowriot]

[slowriot]

[janus]

Great Job.
But I would advise you stick to keccak algo wherever it appears. And before you make a change of such magnitude let me be informed .

[slowriot]

All tests pass:

This PR is ready for acceptance.

[slowriot]

Additional testcases:

[slowriot]

Great Job.
But I would advise you stick to keccak algo wherever it appears. And before you make a change of such magnitude let me be informed .

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.

[janus]

Note that we would not accept this PR. It doesn't bring value to us because it used a Hash algorithm that is not meant for this purpose(BGL).

[slowriot]

Note that we would not accept this PR. It doesn't bring value to us because it used a Hash algorithm that is not meant for this purpose(BGL).

Your response does not make much sense to me. My solution uses precisely the hash algorithm intended for this purpose, as written by the Bitcoin developers who created addrman, with the intention of correctly balancing the address buckets. In fact, an unrelated algorithm had been inserted in its place by a previous contributor - this change failed the bucket balance tests, and rightly so, because that implementation was broken and inappropriate for this use case.

My change repairs the implementation, in line with the Bitcoin core codebase algorithms, and achieves your stated aim - "We want to be as close as possible to Bitcoin codebase.". As a result, the tests all pass.

As discussed at #52 (comment), if you insist on the alternative - taking a broken algorithm, and changing the tests so they pass it, instead of fixing the algorithm so the (already valid) tests pass - then I can do this, but please confirm you are authorised by the project's stakeholders to mandate such a decision; especially considering the security implications of altering the bucket-balancing algorithm in addrman.