1. 04 Aug, 2021 2 commits
    • Calin Culianu's avatar
      Merge branch 'bch-testnet3-seeder' into 'master' · 13368ec1
      Calin Culianu authored
      Add Bitcoin Unlimited operated seeder for testnet3
      
      See merge request !1239
      13368ec1
    • Calin Culianu's avatar
      Update mainnet seeds · 378b2bbe
      Calin Culianu authored
      Summary
      ---
      
      In preparation for a next release, we should update the seed list.
      This MR was prepared by following the instructions in
      `contrib/seeds/README.md`.
      
      The source `dnsseeds.dump` used was from my server at `bchseed.c3-soft.com`
      which has been running for >30 days.
      
      Also in this MR:
      
      - Updated the `makeseeds.py` regex to:
        - No longer accept BCHN 22.x.x for the seeder (22.x.x by now is no longer really
        conforming to latest relay rules).
        - Updated the kth-bch regex for latest knuth
        - Updatede the dnsdump.dump.test file to reflect the changed regexes
          for testing.
      
      Test Plan
      ---
      
      - `ninja all check`
      - Start up a full node **on mainnet**, having made sure to delete its
        peers.dat file first.
        - To ensure that this node uses the seeds and not the DNS seeder, you
          should override `/etc/hosts` on your system to point all the hard-coded
          seeders to 127.0.0.1, so that the node is forced to use the hard-coded seeds
          from this MR, rather than the seeder.
        - Example `/etc/hosts` below:
      
      ```
      127.0.0.1 seed-bch.bitcoinforks.org
      127.0.0.1 btccash-seeder.bitcoinunlimited.info
      127.0.0.1 seed.bchd.cash
      127.0.0.1 seed.bch.loping.net
      127.0.0.1 dnsseed.electroncash.de
      127.0.0.1 bchseed.c3-soft.com
      
      ::1 seed-bch.bitcoinforks.org
      ::1 btccash-seeder.bitcoinunlimited.info
      ::1 seed.bchd.cash
      ::1 seed.bch.loping.net
      ::1 dnsseed.electroncash.de
      ::1 bchseed.c3-soft.com
      ```
      378b2bbe
  2. 03 Aug, 2021 1 commit
    • Calin Culianu's avatar
      update ChainTxData for main, test3, and test4 networks · 188a9b36
      Calin Culianu authored
      Summary
      ---
      
      This data is not used for anything critical other than the Bitcoin-Qt UI
      to display estimated progress info, and also various RPCs that provide information only.
      
      It has been updated for the 3 networks since we will do a release soon.
      
      The tx rate estimate was calculated for by just taking the
      average rate since the last time we did this calculation.
      
      Test Plan
      ---
      
      - `ninja all check`
      - Verify that the changed block height and timestamps match.
      188a9b36
  3. 02 Aug, 2021 2 commits
  4. 24 Jun, 2021 1 commit
  5. 16 Jun, 2021 3 commits
    • Calin Culianu's avatar
      Merge branch 'doc/disclosure_policy_update_1' into 'master' · f9bffda8
      Calin Culianu authored
      [doc] Remove personal disclosure contact
      
      See merge request !1227
      f9bffda8
    • Calin Culianu's avatar
      Nit: Fix compiler warning for g++-8 in txmempool.cpp · c52df0ed
      Calin Culianu authored
      Summary
      ---
      
      When working on other code, I noticed that on Linux on g++-8, I was
      getting the following compiler warning when compiling txmempool.cpp:
      
          ../src/txmempool.cpp: In lambda function:
          ../src/txmempool.cpp:903:20: warning: declaration of ‘const TxId txId’ shadows a parameter [-Wshadow]
                   const TxId txId{txit->GetTx().GetId()};
                              ^~~~
          ../src/txmempool.cpp:893:53: note: shadowed declaration is here
            auto CTxMemPool::recursiveDSProofSearch(const TxId &txId, DspDescendants *desc, double *score) const
                                                   ~~~~~~~~~~~~^~~~
      
      This commit fixes the warning.
      
      Test Plan
      ---
      
      No real changes to this codebase occur here. But:
      
      - `ninja all check check-functional` should do it.
      c52df0ed
    • Calin Culianu's avatar
      Merge branch 'fix/link_common_lib_to_fuzz_targets' into 'master' · 4abadf39
      Calin Culianu authored
      [build] Fix fuzz-libfuzzer undefined references for IsPayToPubKeyHash / IsPayToScriptHash
      
      See merge request !1224
      4abadf39
  6. 15 Jun, 2021 1 commit
    • Calin Culianu's avatar
      Add -`gbtcheckvalidity` arg to CLI;`"checkvalidity"` arg to GBT(L) · fd29ab6c
      Calin Culianu authored
      Summary
      ---
      
      As discussed in Slack, perhaps it would be beneficial to optionally skip
      the slow `TestBlockValidity` checks when generating a block template.
      These checks dominate block template creation for large blocks, and
      in some cases perhaps advanced users (miners, etc) may want to skip these
      checks.  Note that it is a bug in this codebase if we *ever* generate
      an invalid block template anyway, so the checks are largely redundant.
      
      Additoinally, while working on this code, I noticed:
      
      - RPC help for GBT(L) is incomplete. I added the missing `"longpollid"`
      documentation.
      - RPC help for GBT(L) had some redundant pieces. I put the duplicated
      stuff into a common static function `getGBTArgs()`.  This makes it
      easier to maintain the arg code for those two very strongly related functions.
      - I noticed it's annoying when testing that block templates are cached. Perhaps we
      want to skip caching, especially when testing.  I added the optional
      RPC argument to the `template_request` called `"ignorecache"` which, if true,
      always skips the cached block template and generates a new one.
      
      Summary of Changes
      ---
      
      - New arg: `-gbtcheckvalidity` to control the default setting for gbt(l).
        If false, we skip the validity checks when generating a block template
        (default: true)
      - Added help doc for `"longpollid"` `template_request` argument in RPC for
        gbt(l)
      - Added optional `"ignorecache"` `template_request` argument (default:
        false) to skip the cached block template unconditionally (useful for
        testing).
      - Added optional `"checkvalidity"` `template_request` argument (default:
        whatever `-gbtcheckvalidity` was set to on CLI), which controls the
        validity checker on a per-template basis.
      - Refactored the help for `getblocktemplate` and `getblocktemplatelight`
        into a common function.
      - In `checkvalidity=false` mode, also skip the redundant and wasteful
        calculation of block size for the 6th time just to print it to debug log
        (and instead use the estimated worst-case size we already know).
      
      Test Plan
      ---
      
      1. Review
      2. `ninja all check`
      3. `test/functional/test_runner.py bchn-rpc-gbt-checkvalidity-ignorecache`
      fd29ab6c
  7. 14 Jun, 2021 1 commit
    • Calin Culianu's avatar
      Added RPC method `getdsproofscore` · e43045a8
      Calin Culianu authored
      Summary
      ---
      
      This new method is intended to address and close #307.
      The new RPC takes a txid as its only argument and
      returns a number from `0.0` to `1.0`.
      
      - `1.0` indicates no dsproofs exist for this tx or for any of its ancestors,
        but that it and all in-mempool ancestors **can** have a dsproof,
        so confidence should be high that the tx is ok.
      - `0.0` indicates that either this tx or one of its ancestors has a dsproof,
        or that it or one of its in-mempool ancestors can never have a proof
        (not P2PKH) -- so confidence should be low.
      - `0.25` is returned in the case where the tx has so many mempool ancestors
        that no conclusive determination could be made (but the ones that were
        checked are ok).
      - If `txid` doesn't exist in the mempool, the usual `JSONRPCError` is thrown
        (similar to how other RPCs work).
      
      Code Changed
      ---
      
      - Refactored out the code from `compressor.cpp` that checked for p2pkh and p2sh
        and ensured this code (which was somewhat duplicated) all ...
      e43045a8
  8. 13 Jun, 2021 1 commit
    • Calin Culianu's avatar
      Save dsproofs across restarts to datadir/dsproofs.dat · 62c65273
      Calin Culianu authored
      Summary
      ---
      
      This MR adds dsproof saving to disk on node shutdown, and reload
      from disk on node startup.
      
      Motivation: While I was doing some testing and developing I noticed that
      dsproofs get lost across restarts.  Sometimes node admins need to bounce
      nodes and it would be nice if along with the mempool being saved/reloaded,
      the dsproof state were also preserved across restarts.
      
      This will become increasingly important in the future when wallets that
      connect to servers such as Fulcrum start depending on seeing and
      querying the dsproof state of particular transactions.
      
      Description of Change
      ---
      
      The dsproofs get saved to `datadir/dsproofs.dat` in a mechanism similar to
      how the mempool is persisted to `datadir/mempool.dat`.  This is done once on
      node shutdown.
      
      On node restart, this file is loaded before the mempool is loaded and the
      proofs are put into dsproof storage as "orphans".
      
      Should the associated tx for the proof show up in the mempool later when
      the mempool is loaded, the proofs will be re-associated with their
      transactions.
      
      Should the associated tx not ever show up or should it get immediately
      confirmed by new blocks (after a longer period of being offline), the proofs
      are never used and will remain as orphans, to be reaped within 60 seconds
      (normal orphan reaping timeout).
      
      The existing CLI arg `-persistmempool` is consulted when deciding whether
      to save the `dsproofs.dat` file (alongside `mempool.dat`).  This is because
      the two files really go together data-wise (dsproofs are just metadata for
      transactions in the mempool, after all).
      
      Test Plan
      ---
      
      The `bchn-feature-doublespend-proof.py` test was updated to test this new
      mechanism.
      
      - `ninja all check && test/functional/test-runner.py bchn-feature-doublespend-proof`
      - Also, run the above test with `--nocleanup` and look at the `debug.log`
        for `node0`, to observe the new log messages about saving/loading dsproofs
        from disk.
      62c65273
  9. 07 Jun, 2021 2 commits
    • Calin Culianu's avatar
      Nit: Undo some of the changes of core#18710 · 4c6aaf3a
      Calin Culianu authored
      This undoes some of the changes of the core backport #18710, to minimize
      the diff size (keeps more of our code in-tact).
      4c6aaf3a
    • Calin Culianu's avatar
      Remove g_parallel_script_checks global variable · ae2c2fba
      Calin Culianu authored
      This variable is not really used by our codebase -- we always use the
      CCheckQueue, even if no parallel threads are specified (in that case we
      use it with the foreground thread).  So this variable had no meaning in
      our codebase.
      
      This undoes some of the core backport #17342
      ae2c2fba
  10. 06 Jun, 2021 2 commits
    • Calin Culianu's avatar
      Added CLI args to bench: -par and -maxsigcachesize · bac0a4ec
      Calin Culianu authored
      This allows one to run the CCheckQueue_RealBlock_32MB* benches with
      fine-grained control.
      bac0a4ec
    • Calin Culianu's avatar
      bench: Benchmark CCheckQueue using real block data · cc4d85c3
      Calin Culianu authored
      Summary
      ---
      
      While investigating the potential bottlenecks in validating extremely
      large blocks (1 million txns for scalenet), I noticed that the
      CCheckQueue parallel signature validator may need further benchmarking
      and/or potential optimization.
      
      It is a very complicated piece of machinery that also interacts with the
      shared app-wide signature cache under the hood, as such, it could use a
      more realistic benchmark.  The existing benchmark set up a very small
      data set and didn't actually interact with the signature cache, and did
      extremely trivial operations.
      
      This MR is a step towards the goal of better understanding the
      bottlenecks  -- it set ups a more realistic benchmark of the CCheckQueue
      data structure by using block 556034 which is a 32MB block compiled into
      the bench_bitcoin executable.
      
      It then proceeds to actually verify 157783 signatures in parallel, using
      all cores and using the real signature checking code and the read signature
      cache.
      
      We setup 2 different benchmarks:
      
      - one for the case where we *don't* store the sigcheck results to the
      cache -- we simply check sigs in parallel.  This emulates the way IBD
      works (IBD doesn't cache result for blocks as it connects them to the tip,
      since doing so would just be a waste of time since those sigs will never
      be used again once the block is accepted).
      
      - a second benchmark which *does* cache the results of the sigchecks --
      this is closer to how "TestBlockValidity" (called when mining) works. It
      does the sigchecks but stores the results for later possible use.
      
      Test Plan
      ---
      
      - `ninja bench_bitcoin`
      - `src/bench/bench_bitcoin -filter='CCheckQ.*32MB.*'`
      
      Notice how the two different benchmarks run, and how they differ in
      terms of execution speed and also how much total CPU they use. What was
      surprising to me after authoring the two benchmarks is that disabling
      the storage of the cache results actually *sped up* execution on my
      16-core system.  This may be because storing the cached results involves
      grabbing a mutex each time and this potentially seriously slows down the
      parallel signature checker.  This anomaly does deserve further
      investigation as it could be a potential place we can optimize things...
      cc4d85c3
  11. 01 Jun, 2021 1 commit
  12. 25 May, 2021 1 commit
    • Calin Culianu's avatar
      Fix `-txbroadcastinterval=0` leading to no txs sent + add functional test · e877fe48
      Calin Culianu authored
      Summary
      ---
      
      The -txbroadcastinterval=0 argument should mean
      "send txs as fast as possible, without an inv delay".
      For values of -txbroadcastinterval=1 or 2, 4, 100, 500, etc
      the delay keeps increasing, but for 0 it ends up never sending tx invs!
      
      I am pretty sure this is not the intended design.
      
      This MR fixes the issue (explained in #309).
      
      It also adds a functional test to detect if this issue should ever regress.
      
      Test Plan
      ---
      
      - On this branch, do: `ninja all && test/functional/test_runner.py bchn-feature-txbroadcastinterval`
        And observe the test runs ok.
      - Copy the bchn-feature-txbroadcastinterval.py test to a tmp dir, switch to master,
        copy the test back into ../test/functional/.
        And run the test: `test/functional/test_runner.py bchn-feature-txbroadcastinterval`
      
      You should observe it fail off master but run ok off this MR.
      e877fe48
  13. 24 May, 2021 1 commit
    • Calin Culianu's avatar
      Removed remaining unconf. ancestor limit args & logic · 44150d7e
      Calin Culianu authored
      Summary
      ---
      
      This MR builds on top of previous MRs in the interests of completing the tasks
      outlined in #295.
      
      This MR removes the remaining unused logic/code related to the old
      ancestor/descendant limits, including unused args as well as unused code in the
      wallet.
      
      Summary of Changes
      ---
      
      - Removal of: -walletrejectlongchains, -limitdescendantcount, -limitancestorcount, -limitdescendantsize, -limitancestorsize
      - Removal of wallet coin filter logic related to ancestor/descendant size limits
        (the limits weren't even being calculated or used anymore, so it was dead
        code). This simplified the wallet logic a bit.
      - Tests were updated; mainly to remove unused args, but also unit tests were
        updated due to m_ancestors and m_descendants disappearing form the coin filter
        struct(s), etc.
      - Removed the static constants related to the defaults for these limits
        (unused).
      - Minimum mempool size was previously being calculated based off the descendant
        siz...
      44150d7e
  14. 20 May, 2021 3 commits
    • Calin Culianu's avatar
      dsproof: When an orphan is claimed, clear the nodeId · 3944570c
      Calin Culianu authored
      Summary
      ---
      
      This fixes an issue described in #311 where a claimed dsproof orphan
      could lead to a situation where the peer that told you about the proof
      ends up being erroneously punished with a misbehavior score of +1.
      
      The soluton is to clear the dsproof Entry "nodeId" back to -1 when we
      accept a proof and associate it with a real mempool txn.  At this point
      tracking the nodeId (which is only used for banning) is no longer needed
      and it should be cleared to prevent banning later when the dsproof
      completes its lifecycle. Ratonale: if a proof was ever accepeted for any
      reason, banning should become impossible in the future.
      
      Note: The normal lifecycle of a dsproof when its txn confirms is to always
      become an orphan first (so that it cna be kept around in case of reorg),
      before being deleted by the cleaner task after 90 seconds.
      
      Test Plan
      ---
      
      `ninja all check-all`
      3944570c
    • Calin Culianu's avatar
      Remove -tachyonactivationtime from CLI args & consensus rules · 48ce9eea
      Calin Culianu authored
      Summary
      ---
      
      Since this upgrade was relay-rules-only, the activation time is no
      longer relevant for validating the blockchain itself, thus the time
      should no longer be tracked or stored.
      
      Additionally, since the activation logic was removed by a previous MR,
      nothing was using this value in the codebase, and it was just dead code.
      
      Summary of changes:
      
      - removed the CLI arg `-tachyonactivationtime`
      - removed the function IsTachyonEnabled in activation.cpp since nothing
        other than unit tests was calling it.
      - removed the unit test that called this function since it is not needed
      - removed `tachyonActivationTime` from the consensus params
      - left comments in place about what the activation time once was where the
        old data used to live.
      
      Test Plan
      ---
      
      `ninja all check-all && ninja bench_bitcoin`
      48ce9eea
    • Calin Culianu's avatar
      Remove pre-tachyon logic and code from txmempool and related tests · 431374d9
      Calin Culianu authored
      This simplifies the code and removes currently-unused pre-May 15th 2021 code
      paths. For the purposes of mempool policy, we pretend that tachyon always
      existed and we delete all the unused code paths (and stats) that are no
      longer maintained and/or used.
      
      - Removed pre-tachyon support from txmempool and other code
      
      - Undid the delete of the MempoolAncestryTests unit test.
        This unit test, while relying on slow quadratic calculations, is good
        because it does test correctness.
        The slow functions were moved into the mempool_tests.cpp file and are
        just being used for this test, to test correctness.
      
      - Avoid double-copying in ATMP by providing an && overload for addUnchecked
        We provide an rvalue reference overload for CTxMemPool::addUnchecked so
        that we don't have to double-copy the CTxMemPoolEntry coming in from
        AcceptToMemoryPoolWorker
      
      - Removed tachyonLatched from bench/removeforblock.cpp
      
      - Removed limitancestorcount & limitandescendantcount from 2 tests
        These tests were specifying a large ancestor / descendant count so they
        could run.  Since no limit is being enforced anymore, removed these args
        from these tests since they are no longer needed.
      
      - Fixed functional test: mempool_packages.py
        For post-tachyon we simply no longer test the ancestor limit, but invert
        the test such that we test that emitting tx's past the ancestor limit
        works.
      
      - Fixed functional test: mempool_packages_additional.py
        This test was testing the ancestor limits. Now after tachyon they are no
        longer enforced, so the test was updated to instead just exercise the
        code a bit and ensure that the old limit no longer exists.
      
      - Fixed wallet_basic.py test (no longer test chain limits)
        Removed the pre-tachyon testing of wallet & mempool chain limits.
      
      - Fixed functional test: bchn-opreturn.py
        Got rid of activation logic testing -- reduced the test down to simply
        testing for multi-opreturn accept and reject cases.
      
      - Removed pre-tachyon-related unit tests, fixed bench
        All pre-tachyon unit testing that tests ancestor/descendant stats is no
        longer relevant or even tracked by the mempool, so the tests had to go
        (they wouldn't compile anymore anyway).
      
      Test Plan
      ---------
      
      - `ninja all check-all`
      431374d9
  15. 15 May, 2021 2 commits
  16. 14 May, 2021 2 commits
    • Calin Culianu's avatar
      Disable test: bchn-rpc-getblocktemplate-timing.py · cb6b0370
      Calin Culianu authored
      Summary
      ---
      
      This test is extremely unreliable in the CI environment due to the CI
      env being "noisier" timing/CPU-wise (it likely runs in a VPS).
      
      As such, due to the relatively high false failure rate of this test on
      CI, we are better off just disabling it.
      
      A directory was created inside the functional/ test directory called
      "disabled".  We can move tests into here to keep the code around in case
      we want to fix them, but otherwise disable the tests from ever normally
      executing.
      
      Test Plan
      ---
      
      - Observe CI didn't run this `bchn-rpc-getblocktemplate-timing` test.
      cb6b0370
    • Calin Culianu's avatar
      Merge branch 'ej/fixSignRawTransactionWithWalletParam' into 'master' · 58aac283
      Calin Culianu authored
      Fix "hexstring" param name in "signrawtransactionwithwallet" method
      
      See merge request !1206
      58aac283
  17. 13 May, 2021 1 commit
  18. 27 Apr, 2021 1 commit
  19. 26 Apr, 2021 2 commits
    • Calin Culianu's avatar
      Fix for rare failure in the bchn-rpc-getblocktemplatelight functional test · 7e001161
      Calin Culianu authored
      Summary
      ---
      
      This closes issue #300.
      
      Explanation for the failure seen in #300:
      
      It's possible for this functional test to (falsely) fail spuriously due
      to the "cached block template" mechanism interfering with the test.  To
      ensure that it does not, the fix was to call `self.wait_for_txs` for each
      of our two nodes (not just for the first one), which waits until a new
      block template is "ready" and until the old one expires (5s max usually).
      
      With this fix the test should no longer spurously fail.
      
      Test Plan
      ---
      
      - `ninja all && test/functional/test_runner bchn-rpc-getblocktemplatelight`
      
      If you can reliably reproduce the failure,  then by all means try this
      commit and try master and see this commit fixes it.
      
      Due to the timing nature of the failure, it's hard to reproduce. I was
      unable to do so here on my setup, but I am sure of the underlying issue,
      having studied the code again with a critical eye after seeing #300's
      error output.
      7e001161
    • Calin Culianu's avatar
      Merge branch 'qa/nuke_Win32_cmake' into 'master' · d0a91753
      Calin Culianu authored
      [qa] Remove Win32 cmake build file and target reference in doc
      
      See merge request !1196
      d0a91753
  20. 24 Apr, 2021 3 commits
    • Calin Culianu's avatar
      Reduce bchn-rpc-getblocktemplate-timing success threshold to 50% · 3e790599
      Calin Culianu authored
      Summary
      ---
      
      This test is very sensitive to timing and needs to be fixed. Until then,
      this temporary "fix" is to simply reduce the required success threshold
      form 66% to 50%.
      
      It will still fail, but hopefully somewhat less often.
      
      Test Plan
      ---
      
      `ninja && test/functional/test_runner.py bchn-rpc-getblocktemplate-timing`
      3e790599
    • Calin Culianu's avatar
      Ensure the DisconnectedBlockTransactions pool has enough space · 6d174078
      Calin Culianu authored
      Summary
      ---
      
      This commit closes issue #296. In particular, on ScaleNet, the
      statically configured maximum bytes for the disconnect pool is 640MB.
      This is bytes of *dynamic* usage.  ScaleNet, with its block size of
      256MB serialized size -- may exceed this dynamic memory limit (the
      dynamic size multiplier for a tx in the disconnectpool may be ~2.89
      or more of its serializd size).  As such, for ScaleNet, it is unsafe to
      use a disconnectpool that has a max dynamic size of 640MB (see issue #296
      for an explanation as to why).
      
      This commit basically makes the disconnectpool max dynamic size no
      longer be a static constant, but calculated at runtime by checking with
      the global config object (via GetConfig()).
      
      It ensures the size is max(640MB, MaxMempoolSize).  With default settings,
      this change right now only affects scalenet -- for every other network the
      old value of 640MB will continue to be used by default. **Unless**, of course,
      the  user configures a larger mempool size via `-maxmempool=`.
      
      For scalenet, the new value will be the current maxMempoolSize used on scalenet
      (which is 2.5GB by default).
      
      Implementation Details
      ---
      
      A `static` private member was added to `DisconnectedBlockTransactions`
      that calculates the size at runtime, based on current config.
      
      - It just grabs the active config via `GetConfig()`.
        - The alternative would have been to pass down the config object from the
          caller, but I tried that in a branch and the diff was huge.
        - It seemed like overkill as a code change that is only here to fix a minor
          corner case bug discussed in #296.
        - This codebase anyway uses `GetConfig()`, still, all over the place.
      - This function returns `uint64_t` and not `size_t` because it is a
        value that relies on the `GetMaxMempoolSize`, which itself is
        `uint64_t`.
      
      Test Plan
      ---
      
      - `ninja all check-all`
      
      Sadly, there is no test now that tries to reproduce the bug discussed in #296,
      but careful study of the code should indicate that the issue is real and
      not imagined.
      6d174078
    • Calin Culianu's avatar
      Avoid double copy when emplacing into DisconnectedBlockTransactons::txInfo · 2a651f39
      Calin Culianu authored
      Summary
      ---
      
      MR !1128 introduced this code but on second reading I realized the
      double-copy from this MR can be remove by using `try_emplace` and also
      by actually giving the `TxInfo` type a real constructor.
      
      This commit is just a minor code quality commit to reduce the creation
      of a temporary.  In most cases the optimizer will elide this temporary
      anyway, but for debug builds it may not, and also it's of higher code
      quality to just avoid creating it.
      
      Test Plan
      ---
      
      - `ninja all check-all`
      
      No behavioral or other changes are introduced by this commit. This is
      purely a nit/code-quality commit.
      2a651f39
  21. 22 Apr, 2021 3 commits
    • Calin Culianu's avatar
      Prefer emplace to insert in CTxMemPool::addUnchecked for map insertion · d20a0d27
      Calin Culianu authored
      Summary
      ---
      
      This is a small code quality & performance nit.
      
      By using `emplace` instead of `insert`, we avoid the potential creation
      of temporary objects (the `std::pair`) on the stack, and instead we constructs
      the `std::pair` in-place by passing arguments directly to the final allocated
      `std::pair` that lives in the map node.
      
      Test Plan
      ---
      
      No semantic or behavioral changes are introduced.
      
      - `ninja all check`
      d20a0d27
    • Calin Culianu's avatar
      mempool: optimization of removeForBlock + general optimizations · bf6e2316
      Calin Culianu authored
      Summary
      ---
      
      As discussed in issue #161, the performance of `removeForBlock` in particular
      is a critical path that deserves optimization.
      
      This MR significantly improves the performance of `removeForBlock` from 0% to
      as much as 100% or more, depending on the exact mempool layout in question.
      
      The changes introduced also happen to improve the general performance of the
      mempool as well.
      
      Minor RPC Change
      ---
      
      - As a result of these optimizations, the mempool-related RPC calls that
        describe a mempool entry's `spentby` (list of mempool txs that spend a
        particular mempool tx) have a different sort order now. Previously, this list
        would be sorted by txid. Now, it is sorted topologically.
      - `getmempooldescendants` and `getmempoolancestors` now also no longer returns a
        list that is sorted by `TxId` but is rather topologically sorted (by
        `entryId`).
      
      Test Plan
      ---
      
      - `ninja all check-all`
      
      Run these benches on master versus this MR commit:
      
      - `ninja bench_bitcoin && src/bench/bench_bitcoin -filter='(RemoveForBlock.*)|(MempoolAcc.*)|(Reorg.*)|(Generat.*)|(Evict.*)'`
      bf6e2316
    • Calin Culianu's avatar
      Fix randomly failing scheduler_tests test · b1d47fa1
      Calin Culianu authored
      Summary
      ---
      
      This closes issue #266. The root cause is that the original test author
      was misusing threads and/or synchronization primitives. There is no
      guarantee that the first main thread runs before the last scheduled
      task. As such, sometimes, the last task runs and writes "42" into the
      `counter` before the `BOOST_CHECK_EQUAL(counter, 0)` line gets a chance
      to be evaluated in the main thread.
      
      The fix is to not rely on undefined behavior here and instead do things
      properly.  We just save the counter var to a second atomic and check
      everything at the end after the two subordinate tasks have all
      definitely finished running.
      
      Test Plan
      ---
      
      - `ninja all check`
      - Try and reproduce the issue in #266 as described. If you can reproduce
        it against master but not here, then you can be happy this is fixed.
      
      An alternative way to test if you *can't* reproduce the failure against master
      is to:
      
      1. `git checkout master`
      2. Edit `src/test/scheduler_tests.cpp` and insert a
         `std::this_thread::sleep_for(std::chrono::milliseconds(21));` right
      *before* the line in the `schedule_every` test that does
      `BOOST_CHECK_EQUAL(counter, 0);`. This will reproduce the failure every
      time.
      3. `ninja test_bitcoin && src/test/test_bitcoin -t scheduler_tests`
      4. `git checkout THIS_MR_BRANCH`
      5. Add the sleep call in approximately the same place and do steps (2 & 3)
         again.  You should never get a failure now.
      b1d47fa1
  22. 20 Apr, 2021 1 commit
    • Calin Culianu's avatar
      bench: Add removeForBlock benchmark · 305f4021
      Calin Culianu authored
      Summary
      ---
      
      This MR is part of on-going work to optimize
      `CTxMemPool::removeForBlock`, as discussed in issue #161.
      
      As such, it adds a benchmark for this function versus a full mempool of
      ~450k unchained txns.
      
      The bench calls removeForBlock repeatedly with block txns from a ~32MB
      block versus a very full mempool.
      
      Also an 8MB block version is provided as well.
      
      Test Plan
      ---
      
      - `ninja all check bench_bitcoin`
      - `src/bench/bench_bitcoin -filter='RemoveForBlock.*'`
      305f4021
  23. 18 Apr, 2021 1 commit
    • Calin Culianu's avatar
      Fix rare timing-related failures in dsproof functional tests · 4f1cc478
      Calin Culianu authored
      Summary
      ---
      
      As described in issue #291, these tests sometimes fail when they shouldn't.
      The errors turn out to be timing-related. It's possible for e.g. node0
      to send the `firstDSTx` to node1 via the p2p network before we can issue
      the `secondDSTx` to node1 ourselves from the Python side via RPC.  When this
      happens -- the test would throw a `JSONRPCException` for
      `txn-mempool-conflict`.
      
      The fix is to allow for this rare timing-related event and just catch
      the exception and proceed.  In either case (no exception or exception),
      a dsproof will be generated (which is what we are testing for in this
      test).
      
      In order to implement the fix we extended the `TestNode` interface in
      the python test framework with a new method, `call_rpc` which takes an
      optional kwarg `ignore_error=<str>`. If this kwarg is specified, then
      the test will proceed as normal without raising an exception, should
      `<std>` match the expected `JSONRPCException` message.
      
      Closes issue #291.
      
      Test Plan
      ---
      
      - `ninja all check check-functional`
      
      OPTIONAL:
      
      - Try and reproduce the failures described in #291 on master. Then try this
        MR and see it no longer fail.
        - May require a similar setup to #291.
        - A "poor man's hack" for reproducing #291 on a non-slow machine would be
          to add `time.sleep(1)` calls in the bchn-feature-doublespend-proof test
          manually after `firstDSTx` is sent but before `secondDSTx` is sent
          (on master).  Then, try the same with this MR in the same places and see
          it never fail.
      4f1cc478
  24. 17 Apr, 2021 1 commit
    • Calin Culianu's avatar
      mempool: Remove BatchUpdater classes, consolidate & clean-up removeForBlock · 14dcc053
      Calin Culianu authored
      Summary
      ---
      
      As discussed in review and raised in issue #285, the `BatchUpdater`
      class hierarchy is no longer needed. It was added originally to
      abstract-out the logic for confirmed txs being removed from the mempool,
      in order to try out different algorithms.  However, we never ended up making
      use of this abstract interface.  During review of the recent mempool changes,
      it was decided that this interface probably needs to be reverted back to a
      simple direct method inside `CTxMemPool` (for clarity and
      maintainability).
      
      As such, this MR does just that.
      
      Additionally, the unused second parameter `nBlockHeight` to
      `CTxMemPool::removeForBlock()` was removed, in the interests of code
      quality. Should we need this parameter again, we can always bring it
      back.  All call-sites were updated to not pass this argument.
      
      This closes #285.
      
      Test Plan
      ---
      
      - `ninja all check-all`
      14dcc053
  25. 15 Apr, 2021 1 commit
    • Calin Culianu's avatar
      util: Refactor the Defer class to a header to be reusable · 7119bbf0
      Calin Culianu authored
      Summary
      ---
      
      This commit adds a template class called `Defer` to the `util/` subdirectory.
      
      This class is designed to leverage RAII and execute a lambda at scope
      end.
      
      It is used by passing it a lambda (or `std::function`) at construction time
      which it will execute when the `Defer` instance is destructed.  This class can
      be used as a language idiom to defer the execution of cleanup code at scope end
      -- even if an exception is thrown.
      
      Motivation: I noticed in various places in the codebase, very critical
      cleanup code is manually executed at scope end -- however if an
      exception is thrown by the code, the cleanup code will never run thus
      leading to `assert` failures or potential subtle bugs and resource leaks.
      
      What's worse, in some places the critical cleanup code is copy-pased in
      several places.
      
      Using this `Defer` idiom one can escape from all of this pain and also guarantee
      safety.
      
      This class was already used in this codebase in 2 places (added by me
      redundantly in both places, private to each translation unit in which it
      was added).
      
      In this commit we just refactor it out to a publicly-visible header in
      `util/defer.h` and thus we can re-use this idiom everywhere.
      
      Should this MR be accepted and merged, I do plan on using this idiom in
      the future in refactors and code cleanups or in new code.  (I already
      know of 1 place where it should be added for safety.)
      
      Test Plan
      ---
      
      No semantic or other behavioral changes are introduced, however:
      
      - `ninja all check-all`
      7119bbf0