[tests] Update Unit Test for addrman.h/addrman.cpp #10287
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jnewbery
reviewed
May 1, 2017
Contributor
jnewbery
left a comment
jnewbery
left a comment
There was a problem hiding this comment.
ACK, with a question about the modulo behaviour in test 25.
src/test/addrman_tests.cpp
Outdated
Contributor
There was a problem hiding this comment.
This code isn't at all doing what I expected. On first looking, I assumed it was slicing off 8 bits at a time in order to format a 32 bit int as an ipv4 address, ie I was expecting this:
int octet1 = i % 256;
int octet2 = i >> 8 % 256;
int octet3 = i >> 16 % 256;
but in fact it's effectively doing this:
int octet1 = i % 256;
int octet2 = i >> 8 % 256;
int octet3 = i >> 9 % 256;
ie the bits used for octet 2 and 3 are overlapping. Did you work out whether there was a reason for this?
I'd prefer to keep these using the modulos to indicate that each octet should be in the range 0-255. The only reason your suggested change would work is because i never exceeds 2^16. Perhaps replace with the following if there's no good reason to keep the strange octet3 behaviour:
int octet1 = i % 256;
int octet2 = i >> 8 % 256;
std::string strAddr = boost::to_string(octet1) + "." + boost::to_string(octet2) + ".0.23";
ie eliminate octet3 entirely.
Contributor
Author
There was a problem hiding this comment.
I was expecting the same until I tried to refactor it. I can't speak to the original intent, but @EthanHeilman can maybe give us a clue as to what it was?
Anyway it turns out that the addresses are going into a hash table and any adjustment of octet3 in any way changes the numbers in lines 415-417 (vAddr size and addrman size). Thankfully, the hash table is deterministic, but I wanted minimal disruption at least for this PR. I was thinking that this test could be refactored slightly so we can predict the number of collisions in another PR.
Contributor
There was a problem hiding this comment.
@jimmysong It just makes it less likely we will have a collision when adding addresses.
Changing
int octet3 = (i / (256 * 2)) % 256;to
int octet3 = (i / (256 * 256)) % 256;would cause all of these generated addresses to map to the same source group in the new table as they would all have the same \16.
Current behavior is intended to lower the chance that addresses are evicted when adding them by spreading them across more than one source group. A byproduct of this is also a more realistic test since the addresses returned by GetAddr() are now being drawn from several source groups.
Contributor
There was a problem hiding this comment.
Can this be achieved by fixing octet2 to zero and then setting octet3 to i >> 8 % 256?
Contributor
There was a problem hiding this comment.
Yes, that would be a better way of doing it.
Contributor
There was a problem hiding this comment.
Not sure if it's better, but I think it would be less confusing :)
Contributor
There was a problem hiding this comment.
Less confusing is better. =)
Contributor
Author
|
@EthanHeilman @jnewbery, I've tested a few combinations, and leaving out octet3 works for the current code I have. It's almost exactly the same as before. Let me know what you guys think. |
Contributor
|
Removing octet3 seems reasonable and the code looks better especially when with the slick BOOST_CHECK_EQUAL. I've been regretting numbering these tests, e.g. "Test 1: , Test: 2 ...", for a while since adding new tests requires renumbering them or using decimals as done here: // Test 6.5: AddrMan::Add multiple addresses works as expectedCould you do a commit which just removes the numbers so "Test 1:" becomes "Test: "? |
Contributor
Author
|
@EthanHeilman sure, will work on changing to BOOST_CHECK_EQUALS and taking out test numbers as soon as this one gets merged. |
Contributor
Alternatively, you could replace the comments with checkpoints: |
jnewbery
reviewed
May 2, 2017
src/test/addrman_tests.cpp
Outdated
Contributor
There was a problem hiding this comment.
nit: I still think this is clearer if you bitshift and modulo 256.
Add test for adding multiple addresses to address manager Clean up unnecessary modulo operations Add test for GetNewBucket's alternate method signature
Contributor
Author
|
nits addressed, squashed and rebased. |
Contributor
|
tested ACK ed36de5 |
maflcko
pushed a commit
that referenced
this pull request
May 9, 2017
ed36de5 [tests] Update Unit Test for addrman.h/addrman.cpp (Jimmy Song) Tree-SHA512: e7c08c19e227c34c230900e14a176b2290022b78b0ece387452e673662491c11f26249cbf1711235276c07a964c339e27b4cda9a2730ded5c0e23a650e0d72db
Member
|
utACK ed36de5 |
jimmysong
added a commit
to jimmysong/bitcoin
that referenced
this pull request
May 9, 2017
Cleanup request from bitcoin#10287. Change "Test #:" comments to "Test:" Change BOOST_CHECK(... = ...) to BOOST_CHECK_EQUAL(..., ...) Remove three unnecessary if statements
CryptAxe
added a commit
to CryptAxe/bitcoin
that referenced
this pull request
Jun 20, 2017
* Switch CCoinsMap from boost to std unordered_map
* Fix some empty vector references
streams.h has some methods that can be tricked into dereferencing
null pointers or end() iterators. Fix this.
* wallet: Add comment describing the various classes in walletdb.h
* contrib: github-merge improvements
Some minor github-merge improvements I've made over time:
User interface:
- Print merge details again before signing off, to refresh your memory -
usually I'll have done lots of different things in the shell so this
will have scrolled out a long time ago.
- Require a valid answer on the prompts. One of the requested answers
must be typed, if not, the prompt will re-ask. This prevents
accidentally rejecting.
Efficiency:
- Condense "accept merge" and "sign off" prompts. There's no reason to
have this as two separate prompts, both are just opportunities to skip
out on the merge, no action is performed in between.
Merging:
- Strip spaces from github title. This avoids redundant spaces
surrounding it from getting into the commit message.
* Tests: Add test for getnetworkhashps
* Fixed typo in documentation for merkleblock.h
* doc: Add historical release notes for 0.14.1
* [doc] Minor corrections to osx dependencies
* Trivial: fix fee estimate write error log message
* Remove Clang workaround for Boost 1.46
* Prevent warning: variable 'x' is uninitialized
* [wallet] Make sure pindex is non-null before possibly referencing in LogPrintf call.
* Remove unused variable from `macdeployqtplus` script.
* Use `with` in `macdeployqtplus` script.
* [test] Add Unit Test for GetListenPort
Add very basic unit test for GetListenPort in net_tests.cpp
* [wallet] Add GetLegacyBalance method to simplify getbalance RPC
This adds a simpler new implementation of getbalance logic along with asserts
to confirm it behaves identically to the old logic. The old logic is removed in
the next commit.
* [wallet] Remove unneeded legacy getbalance code
* [test] Unit test amount.h/amount.cpp
Add tests for MoneyRange, binary operators, ToString and a constructor
* Remove unused C++ code not covered by unit tests
* cleanup: reduce to one GetMinimumFee call signature
* Add -stopatheight for benchmarking
* Improved efficiency in COutPoint constructors
* refactor TxToJSON() and ScriptPubKeyToJSON()
* [net] listbanned RPC and QT should show correct banned subnets
* [tests] update disconnect_ban.py test case to work with listbanned
* [test] Add tests for getconnectioncount, getnettotals and ping
* Split run_test into 4 separate tests
* Add 2 tests, getconnectioncount and getnettotals
* getnettotals - Strategy of test is to get the network stats before and after a ping. The difference in bytes sent/received is the bytes needed for a ping/pong.
* [test] Add gettxout call
Test gettxout as part of the wallet test.
Tests gettxout with a confirmed/unconfirmed tx with include_mempool flag on and off
* Simplify DisconnectBlock arguments/return value
DisconnectBlock currently has a complicated interface:
Situation Return value
pfClean != nullptr pfClean == nullptr
All good: true true
Failure: false false
Unclean rewind: true false
with *pfClean=false
Change this to return a tristate enum instead. As an added bonus,
remove the ValidationState& argument which was unused.
* [Makefile] Alphabetically Reorder addrdb.cpp
To keep conformity.
* [Wallet] unset change position when there is no change on exact match
* Fix potential NPD introduced in b297426
See bitcoin#10290 (comment)
for more info.
* [test] Add test for listaddressgroupings
Test added as part of wallet-accounts.py.
Make file a little more flake8 compliant
* [tests] allow zmq test to be run in out-of-tree builds
* Trivial: remove extra character from comment
* doc: Add RPC interface guidelines
* Remove unused args from GetFetchhFlags()
* Remove unused forward declaration for non-existent ScriptPubKeyToJSON(...)
* [tests] Remove is_network_split from funtional test cases
* [tests] Update Unit Test for addrman.h/addrman.cpp
Add test for adding multiple addresses to address manager
Clean up unnecessary modulo operations
Add test for GetNewBucket's alternate method signature
* move initialize_chain() and initialize_chain_clean() to be methods of BitcoinTestFramework
* Add start and stop node methods to BitcoinTestFramework
* Remove unused Python imports
* Remove unused argument from MarkBlockAsInFlight(...)
* Add getchaintxstats RPC
* [depends] Latest config.guess and config.sub
* [depends] Boost 1.64.0
* [depends] libevent 2.1.8-stable
* [depends] ccache 3.3.4
* [depends] dbus 1.10.18
* [tests] fix wait_for_inv()
* [tests] remove import-abort-rescan.py
Reverts PR 10225
* Update contrib/debian to latest Ubuntu PPA upload.
This:
* Partially reverts 9f68ed6 (which fixed spelling in a changelog,
though generally changelogs should be append-only).
* Disables UPnP support (PPA has not had it for a while, and I
still don't trust miniupnpc, plus it seems uneccessary - its
been a while since we needed to care about Bitcoin-Qt home users
getting their inbound ports auto-mapped).
* Enables ZMQ.
* Forces GUI to Qt4 to fix various issues people have been seeing
on Ubuntu and elsewhere with Qt5.
* Reverts 70899d7 (Bitcoin does not enable "instant payments",
not is transaction management "carried out collectively by the
network", for whatever "transaction management" means, finally
Bitcoin Core is not the only way to use the Bitcoin currency,
as seemingly implied in the description).
* Consensus: Minimal way to move dust out of consensus
* Consensus: Policy: MOVEONLY: Move CFeeRate out of the consensus module
...from amount.o to policy/feerate.o
Policy, because it moves policy code to the policy directory (common module)
* Chainparams: Use a regular factory for creating chainparams
* Chainparams: Get rid of CChainParams& Params(std::string)
* Chainparams: Use the factory for pow tests
* [wallet] Securely erase potentially sensitive keys/values
* net: make CNode's id private
* scripted-diff: net: Use accessor rather than node's id directly
-BEGIN VERIFY SCRIPT-
sed -i "s/\(node\|to\|from\)->id/\1->GetId()/" src/net.cpp src/net_processing.cpp
-END VERIFY SCRIPT-
* devtools: add script to verify scriptable changes
* Maintain state across GetStrongRandBytes calls
* Bump minimum boost version in contrib/debian
* rpc/wallet: Workaround older UniValue which returns a std::string temporary for get_str
* [tests] Fix abandonconflict.py intermittency
* Build with QT5 on Debian-based systems using contrib/debian
* Re-enable upnp support in contrib/debian
* Use hardware timestamps in RNG seeding
* Test that GetPerformanceCounter() increments
* Use sanity check timestamps as entropy
* Reorganize BitcoinTestFramework class
* [doc] Add hint about getmempoolentry to getrawmempool help.
* [qa] Fixes segwit block relay test after inv-direct-fetch was disabled
This test was passing because we never fetch blocks if we only receive
an inv and not the header (after 037159c),
and this test wasn't delivering the header.
* Bugfix: PrioritiseTransaction updates the mempool tx counter
The mempool's nTransactionsUpdated is used by getblocktemplate
to trigger new invocations of CreateNewBlock().
* [qa] Test prioritise_transaction / getblocktemplate interaction
* removed unused code in INV message
vToFetch is never used after declaration. When checked if not empty,
evaluation is always false. Best case scenario this is optimized by the
compiler, worst case it wastes cpu cycles. It should be removed either
way.
* test: Add elapsed time to RPC tracing
Add elapsed time to output of `--tracerpc`. To find out why tests are
slow.
* [tests] use wait_until in mempool_persist.py
* qa: disablewallet: Check that wallet is really disabled
* Add OSX keystroke to clear RPCConsole
Currently only Ctrl-L is mentioned in help, but, (⌘)-L functions on OSX and isn't mentioned.
* [tests] Make wait_until timeout 60 seconds by default
* [tests] increase timeouts in sendheaders test
* Util: Create ArgsManager class...
- Introduce ArgsManager::GetArgs()
- Adapt util_tests.cpp to ArgsManager
* scripted-diff: Util: Encapsulate mapMultiArgs behind gArgs
-BEGIN VERIFY SCRIPT-
sed -i 's/mapMultiArgs.count(/gArgs.IsArgSet(/g' ./src/*.h ./src/*.cpp ./src/*/*.h ./src/*/*.cpp ./src/*/*/*.h ./src/*/*/*.cpp ;
sed -i 's/mapMultiArgs.at("/gArgs.GetArgs("/g' ./src/*.h ./src/*.cpp ./src/*/*.h ./src/*/*.cpp ./src/*/*/*.h ./src/*/*/*.cpp ;
-END VERIFY SCRIPT-
* Util: Put mapMultiArgs inside ArgsManager
- Set ArgsManager::mapMultiArgs in ArgsManager::SoftSetArg, ForceSetArg, SoftSetBoolArg
* Util: Small improvements in gArgs usage
- Don't check gArgs.IsArgSet() is greater than 0
- Remove unneeded calls and local variables
* [tests] Clean up addrman_tests.cpp
Cleanup request from bitcoin#10287.
Change "Test #:" comments to "Test:"
Change BOOST_CHECK(... = ...) to BOOST_CHECK_EQUAL(..., ...)
Remove three unnecessary if statements
* [tests] fix disconnect_ban intermittency
* [test] Add test for getmemoryinfo
Checks memory before and after a transaction that requires a private key.
Each time, 32 bytes of memory for a private key should be used.
Tested in wallet.py instead of its own file to save testing time.
* [doc] Removing comments about dirty entries on txmempool
* Expose estimaterawfee
Track information the ranges of fee rates that were used to calculate the fee estimates (the last range of fee rates in which the data points met the threshold and the first to fail) and provide an RPC call to return this information.
* minor refactor: explicitly track start of new bucket range and don't update curNearBucket on final loop.
* Refactor to update moving average on fly
* Make EstimateMedianVal smarter about small failures.
Instead of stopping if it encounters a "sufficient" number of transactions which don't meet the threshold for being confirmed within the target, it keeps looking to add more transactions to see if there is a temporary blip in the data. This allows a smaller number of required data points.
* Change parameters for fee estimation and estimates on all 3 time horizons.
Make feerate buckets smaller (5% instead of 10%) and make the 3 different horizons have half lifes of 3 hours, 1 day and 1 week respectively.
* Track failures in fee estimation.
Start tracking transactions which fail to confirm within the target and are then evicted or otherwise leave mempool.
Fix slight error in unit test.
* Rewrite estimateSmartFee
Change the logic of estimateSmartFee to check a 60% threshold at half the target, a 85% threshold at the target and a 95% threshold at double the target. Always check the shortest time horizon possible and ensure that estimates are monotonically decreasing. Add a conservative mode, which makes sure that the 95% threshold is also met at longer time horizons as well.
* Track first recorded height
Track the first time we seen txs in a block that we have been tracking in our mempool. Used to evaluate validity of fee estimates for different targets.
* Clean up fee estimate debug printing
* Historical block span
Store in fee estimate file the block span for which we were tracking estimates, so we know what targets we can successfully evaluate with the data in the file. When restarting use either this historical block span to set valid range of targets until our current span of tracking estimates is just as long.
* Introduce a scale factor
For the per confirmation number tracking of data, introduce a scale factor so that in the longer horizones confirmations are bucketed together at a resolution of the scale. (instead of 1008 individual data points for each fee bucket, have 42 data points each covering 24 different confirmation values.. (1-24), (25-48), etc.. )
* minor cleanup: remove unnecessary variable
* Comments and improved documentation
* Shadowing is not enabled by default, update doc accordingly.
* [logging] log system time and mock time
* [Qt] simple fee bumper with user verification
* Add cs_wallet lock assertion to SignTransaction()
* Show old-fee, increase a new-fee in Qt fee bumper confirmation dialog
* Make sure we always update the table row after a bumpfee call
* Make sure we use nTxConfirmTarget during Qt fee bumps
* Only update the transactionrecord if the fee bump has been commited
* Make sure we re-check the conditions of a feebump during commit
* Output line to debug.log when IsInitialBlockDownload latches to false
* Use a verbosity instead of two verbose parameters
Verbose is changed to an int. This can have values from 0-2 for each level of verbosity.
Verbosity level 2 has transaction details displayed in the results.
* Replace boost::function with std::function (C++11)
* qa: Warn when specified test is not found
* Add logging to FinalizeNode()
* tests: Correct testcase in script_tests.json for large number OP_EQUAL
Fix a test case that was passing correctly by accident, but not testing
the right thing. Reported by helo on IRC.
* torcontrol: Improve comments
* torcontrol: Add unit tests for Tor reply parsers
* torcontrol: Fix ParseTorReplyMapping
- Ignore remaining input if it is an OptArguments
- Correctly handle escapes
* torcontrol: Check for reading errors in ReadBinaryFile
This ensures that ReadBinaryFile never returns exactly TOR_COOKIE_SIZE bytes if
the file was larger than that.
* torcontrol: Log invalid parameters in Tor reply strings where meaningful
* torcontrol: Handle escapes in Tor QuotedStrings
https://trac.torproject.org/projects/tor/ticket/14999 is tracking an encoding
bug with the Tor control protocol, where many of the QuotedString instances that
Tor outputs are in fact CStrings, but it is not documented which ones are which.
https://spec.torproject.org/control-spec section 2.1.1 provides a future-proofed
rule for handing QuotedStrings, which this commit implements.
This commit merges all six commits from zcash/zcash#2251
* Fix docs (there's no rpc command setpaytxfee)
* Add query options to listunspent rpc call
* [depends] miniupnpc 2.0.20170509
* Add witness data output to TxInError messages
* Expand signrawtransaction.py to cover error witness checking
* Make more json-like output from estimaterawfee
* [tests] improve tmpdir structure
* tests: fix spurious addrman test failure
When inserting two addresses of the same class, from the same source, they have
a 1/64 chance of colliding.
* Prevent shadowing the global dustRelayFee.
* [Trivial] Add BITCOIN_FS_H endif footer in fs.h
* Add core components of Drivechains and BMM
* Add SCDB to track sidechain WT^ work score
* Add coinbase cache to track N recent coinbases. Used both to synchronize SCDB and BMM linking data
* Add OP_BRIBE and OP_BRIBE activation
* Add wallet function to create sidechain desposits which have unique requirements
* Miner broadcasts WT^ in block if workscore is made sufficient
* CheckWorkScore code
* Initialize everything during AppInit
* Added sidechain address prefix to be used
* Added testing list of valid sidechains test, hivemind, mimble wimble
* Add SCDB and BMM RPC calls
* receivesidechainwtjoin
* listsidechaindeposits
* getbmmproof
* createbribe
* refundbribe
* Add some basic SCDB tests, update json & py
* Add GUI elements of Drivechains
* Add sidechain deposit button to sendcoinsdialog
* Add sidechain deposit dialog
* Update comments & sidechain deposit / withdraw
* Add GetBWTHash() function to CTransaction so that we don't have to manually
blind the transaction when checking work score.
* Update comments to make them more clear and add TODO messages about what
still needs to be finished in AcceptToMemoryPoolWorker().
* Update comments & code in connect block where WT^ workscore is
verified
* RPC, script, unit test updates for OP_BRIBE & BMM
* Update rpc client param conversion
* Update createbribe rpc function (WIP)
* Update IsBribe()
* Add BMM ratchet unit tests
* Enforce BMM ratchet block number rules
* rename all SidechainDB class according to bitcoin style guide (#10)
* Revert "rename all SidechainDB class according to bitcoin style guide (#10)" (#12)
This reverts commit 4eb5c1d.
* Remove comment that is now incorrect
* Update README.md
HashUnlimited
pushed a commit
to chaincoin/chaincoin
that referenced
this pull request
Mar 2, 2018
Cleanup request from bitcoin#10287. Change "Test #:" comments to "Test:" Change BOOST_CHECK(... = ...) to BOOST_CHECK_EQUAL(..., ...) Remove three unnecessary if statements
PastaPastaPasta
pushed a commit
to PastaPastaPasta/dash
that referenced
this pull request
Jun 19, 2019
ed36de5 [tests] Update Unit Test for addrman.h/addrman.cpp (Jimmy Song) Tree-SHA512: e7c08c19e227c34c230900e14a176b2290022b78b0ece387452e673662491c11f26249cbf1711235276c07a964c339e27b4cda9a2730ded5c0e23a650e0d72db
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
5 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Add test for adding multiple addresses to address manager
Clean up unnecessary modulo operations
Add test for GetNewBucket's alternate method signature