notes.txt

NOTES

-----

wumpus:   Focus on user problems, actual bugs, and "used, but untested" methods
          that affect outcomes and need tests

          What kind of frustrates me, personally, is that we didn't manage to
          clearly separate out the consensus parts. Not to a library (which
          would be useful for rust-bitcoin et al), but also not even to a
          separate directory.

          I think it's important to isolate the consensus code soon, and be
          extremely careful about that. I'm not sure we can be careful
          enough. yes, I think isolating the conensus code would be most bang
          for buck with regard to risk minimalization. make it clear which areas
          of the code are the really risky ones that could break bitcoin as a
          thing. bitcoin core is too big.

sipa:     happy to see renewed activity on that front with dongcarl's recent PRs

wumpus:   but I've been screaming into the void about this since forever, I'm
          happy dongcarl picked up on it.
-----

jnewbery: Review 5-10 PRs per PR made
          3-4 ACKs before merging is good
          Start small
          Have a plan, don't spread yourself too thin
          Sharpen your tools
          Ask for and offer help, like rebasing for people or adding test cases
          People are generous with their time because they care
          Contribute by understanding what others want and being respectful
          Learn to get people to review when needed
-----

          To grep Bitcoin "hidden" debug options:
          bitcoind -help-debug | grep -A3 -- -limit
-----

          Use Clang formatting to clean up a file:
          clang-format -i <file>

          To format the last commit with 0 lines of context, call the
          clang-format-diff.py script from root as follows:
          git diff -U0 HEAD~1.. | ./contrib/devtools/clang-format-diff.py -p1 -i -v
-----

          To test compiler acceptance on the command line:
          echo 'int main() {  }' | g++ -x c++ -

          Examples:
          echo 'int main(){ const int min_depth = { true ? 1 : 0 };}' | g++ -x c++ -
          echo 'int main(){ const int min_depth   { true ? 1 : 0 };}' | g++ -x c++ -
          echo 'int main(){ const int min_depth =   true ? 1 : 0  ;}' | g++ -x c++ -
          echo 'int main(){ const int min_depth =   true ? 1 : 0   }' | g++ -x c++ -
          <stdin>: In function ‘int main()’:
          <stdin>:1:51: error: expected ‘,’ or ‘;’ before ‘}’ token
-----

practicalswift: C++ undefined, unspecified and implementation-defined behavior:

          - undefined: "behavior, upon use of a nonportable or erroneous program
            construct or of erroneous data, for which this International
            Standard imposes no requirements."

          - unspecified: "use of an unspecified value, or other behavior where
            this International Standard provides two or more possibilities and
            imposes no further requirements on which is chosen in any instance."

          - implementation-defined: "unspecified behavior where each
            implementation documents how the choice is made."
-----

          Git stuff

          To search for a term in git history:
          git log -S 'some string'

          To see the git log of a function over time:
          git log -L:function:file.c

          This works for any line range as well:
          git log -L 120,130:file.h

          To search git history for a GitHub PR number:
          git log --grep='#1117 ' --merges

          To see moved-only code:
          git show --color-moved=dimmed-zebra

          Word diff:
          git diff --word-diff

          Git diff that ignores all added lines having a matching identical
          removed line. An empty result proves that a diff is only moving lines:
          git diff 3f95fb0~1..3f95fb0 | grep -E '^[+-][^+-]' | cut -b2- | sort \
          | uniq -c | grep -v '^ *2'

          git grep -iE '[^0-9a-z]520[^0-9a-z]' -- '*.cpp' '*.h'
          grep -rnw './depends' -e 'http:'

          git grep -l VARINT | xargs sed -i 's/VARINT(\([^,]\+\), VarIntMode::DEFAULT)/VARINT(\1\)/'
          git diff HEAD^

          git range-diff master PREV NEW

          PREV=317bf6a NEW=ab8a6cd && diff <(git show $PREV) <(git show $NEW)

          git blame src/wallet/rpcwallet.cpp | cut -c 39-65 | sort | uniq -c | sort -g

          grepping rpc output:
          bitcoin-cli getpeerinfo | grep 'banscore' | sort

          To check each commit when feeling patient:
          git rebase -i $(git merge-base HEAD origin/master)"
          change all the picks to edit, then
          "make -j12 && make -j5 check && (cd ../test/functional;
          ./test_runner.py) && git rebase --continue

          and try this to have all of those edits filled for you:
          git rebase -i $(git merge-base HEAD master) -x "<command>"

          Verify a revert
          from https://github.com/bitcoin/bitcoin/pull/18588#pullrequestreview-391760193
          git checkout $(git merge-base origin/master origin/pull/18588/head)
          git revert --no-edit $(git rev-list --min-parents=2 --max-count=1 origin/pull/16367/head)..origin/pull/16367/head
          git diff origin/pull/18588/head
-----

          Unit tests

          Info: https://github.com/bitcoin/bitcoin/blob/master/src/test/README.md

          Help: test_bitcoin --help
          or:   src/test/test_bitcoin --help

          Run all tests with make check, test_bitcoin, or src/test/test_bitcoin

          To run a suite: test_bitcoin -t suite_name   (using -t or --run_test=)
          (for example)   test_bitcoin -t wallet_tests
          or              test_bitcoin -t ismine_tests

          To run a test:  test_bitcoin -t suite_name/case_name
          (for example)   test_bitcoin -t wallet_tests/ListCoins
                          test_bitcoin -t ismine_tests/ismine_standard

          Run QT unit tests with: src/qt/test/test_bitcoin-qt

          Specify the log level with -l or --log_level= (see --help).
          Options: all|success|test_suite|unit_scope|message|warning|error|
                   cpp_exception|system_error|fatal_error|nothing
          Most useful: all, success, test_suite

          The test logs to a debug log file (in master)
          If you need temporary printing, you can do std::cout or std::cerr
          or BOOST_TEST_MESSAGE();

          ryanofsky tip to get full debug log of test run:
          test_bitcoin -l test_suite -- DEBUG_LOG_OUT
-----

          Functional tests

          To profile:  python3 -m cProfile -s time feature_block.py

          To run a functional test X times and print the run times:
          for i in {1..20}; do ./test/functional/mempool_updatefromblock.py | grep -o 'in [0-9]\.[0-9]*'; done


          PDB (Python debugger)

          Add break points to manually run RPC commands with:
          import pdb; pdb.set_trace()

          To attach a python debugger if test fails, run the test and pass --pdbonfailure:
          test/functional/wallet_basic.py --pdbonfailure

          Does not work via the test runner, e.g. this prints dots ad infinitum:
          test/functional/test_runner.py wallet_basic --pdbonfailure

          To run a functional test X times and launch the debugger on failure:
          (for i in {1..100}; do test/functional/wallet_basic.py --pdbonfailure ; done)

          Put error messages in the assertion instead of print to stdout. See:
          https://github.com/bitcoin/bitcoin/pull/18516#discussion_r404191587
          raise AssertionError("Error: Invalid vsize of {}".format(tx["vsize"]))

          ./src/qt/bitcoin-qt -lang=nl -blocksdir=/tmp/no/no -printtoconsole | grep 'no/no' 2019-06-15T10:11:27Z Fout: Opgegeven blocks map "/tmp/no/no" bestaat niet.

          To run functional tests via the gui from the command line, pass BITCOIND=bitcoin-qt

          LC_ALL=de_DE.UTF-8 QT_QPA_PLATFORM=minimal BITCOIND=bitcoin-qt ./test/functional/test_runner.py  --tracerpc feature_versionbits_warning.py

          LC_ALL=de_DE.UTF-8 QT_QPA_PLATFORM=xcb BITCOIND=bitcoin-qt ./test/functional/p2p_permissions.py --tracerpc
-----

          Fuzz tests

          Help: pass with -help=1 argument:
          src/test/fuzz/process_messages -help=1

          Documentation
          https://llvm.org/docs/LibFuzzer.html

          Source code
          https://llvm.org/doxygen/FuzzerMutate_8h_source.html

          Compile with Clang:
          CC=clang CXX=clang++ ./configure --enable-fuzz --with-sanitizers=address,fuzzer,undefined
          May need to run `make distclean` first.

          Run an individual fuzz test:
          src/test/fuzz/base_encode_decode

          You can supply a folder where to put the seeds:
          mkdir -p process_messages/
          src/test/fuzz/process_messages process_messages/

          If you inspect the seeds manually with cat, be sure to pass in
          `cat --show-nonprinting` otherwise it might execute arbitary code
          in your terminal:
          cat --show-nonprinting ./where/to/put/the/seeds/000fff

          Run all fuzz tests with the test runner (not working; for the CI):
          ./test/fuzz/test_runner.py
-----

          Memory profiling:

          Use valgrind + massif_visualizer, ideally in a controlled environment,
          e.g. regtest, a small block chain, a custom wallet, etc. Valgrind is
          much more precise, fine-grained analysis than a handmade python
          procutils script (at the cost of being slow).

          strace - man page strace

          With these tools, run them in one terminal buffer, while performing
          operations under test (say, loading/unloading the wallet) in another
          terminal buffer.
-----

          Bench:

          ./src/bench/bench_bitcoin -evals=1
          ./src/bench/bench_bitcoin -filter=VerifyNestedIfScript -evals=50
          ./src/bench/bench_bitcoin -filter="SipHash|SipHash_3b|SipHash_32b"

          git clean -dfX && git checkout master && ./autogen.sh && ./configure --with-incompatible-bdb --without-gui && make -j5 && ./src/bench/bench_bitcoin --filter='(SHA256|Merkle).*'

          git clean -dfX && ./autogen.sh && ./configure --with-incompatible-bdb --without-gui && make -j5 -C src bench/bench_bitcoin && ./src/bench/bench_bitcoin --filter='(SHA256|Merkle).*'

gwillen   I suggest -dfX instead of -dfx (capitalize the x) when doing git clean
          as it may save you someday. X only removes ignored files, x also
          removes locally added non-ignored files, including new code you did
          not yet call 'git add' on.

-----

          Commit stats bash scripts

          Count non-merge commits over a time period
          git log --oneline --since=2019-01-01 --until=2019-12-31 --no-merges | wc -l

          Commits/contributor ranking over a time period
          git log --since=2020-01-01 --until=2020-12-31 --no-merges \
          | grep '^Author' | sort | uniq -c | sort -nr | head -n 40

          Count contributors over a time period
          git log --since=2019-01-01 --until=2019-12-31 --no-merges \
          | grep '^Author' | sort | uniq -c | sort -nr | wc -l

          List top 20 contributors in commits per year
          for i in 2012 2013 2014 2015 2016 2017 2018 2019 2020; do echo $i \
          `git log --since=$i-01-01 --until=$((i+1))-03-15 --no-merges \
          | grep '^Author' | cut -d':' -f2- | cut -d'<' -f1 | sort | uniq -c \
          | sort -nr | head -n 20 | awk '{print $2}' | tr '\n' ' '` ; done
-----

vasild    https://github.com/bitcoin/bitcoin/pull/19551/files
          for "unused variable" warnings like your PR, can also do:

          auto f = [] {
              std::cout << "foo" << std::endl;
              return true;
          };
          f();

          or

          [] {
              std::cout << "foo" << std::endl;
              return true;
          }();
-----

          Valgrind Memcheck incantations (WIP)

          make check-valgrind


Run to the end and generate suppressions:

valgrind --leak-check=full --show-leak-kinds=all --show-reachable=yes --gen-suppressions=all --error-limit=no -s --suppressions=contrib/valgrind.supp src/test/test_bitcoin --log_level=test_suite

Exit on first error:

valgrind --leak-check=full --show-leak-kinds=all --show-reachable=yes --gen-suppressions=all --exit-on-first-error=yes --error-exitcode=1 -s --suppressions=contrib/valgrind.supp src/test/test_bitcoin --log_level=test_suite



valgrind --gen-suppressions=all --quiet --error-exitcode=1 --suppressions=contrib/valgrind.supp src/test/test_bitcoin -t util_tests/test_LockDirectory

valgrind --gen-suppressions=all --quiet --error-exitcode=1 --suppressions=contrib/valgrind.supp src/test/test_bitcoin -t ismine_tests/ismine_standard


valgrind --gen-suppressions=all --error-exitcode=1 --suppressions=contrib/valgrind.supp src/test/test_bitcoin -t wallet_tests --log_level=test_suite

valgrind --gen-suppressions=all --verbose --exit-on-first-error=yes --error-exitcode=1 --suppressions=contrib/valgrind.supp src/test/test_bitcoin -t wallet_tests --log_level=test_suite


valgrind --gen-suppressions=all --quiet --error-exitcode=1 --suppressions=contrib/valgrind.supp src/test/test_bitcoin -t wallet_tests/ListCoins

Best so far, good for running all tests:

valgrind --gen-suppressions=all --verbose --exit-on-first-error=yes --error-exitcode=1 --suppressions=contrib/valgrind.supp --leak-check=full --show-leak-kinds=all src/test/test_bitcoin --log_level=test_suite

valgrind --gen-suppressions=all --verbose --exit-on-first-error=yes --error-exitcode=1 --suppressions=contrib/valgrind.supp src/test/test_bitcoin --log_level=test_suite

-----
          GDB

          cd src
          gdb bitcoind
          b bitcoin-cli.cpp:246
          start -regtest

          gdb - args
          b <function>
          r
          bt
          n (next)
          p (print) tx, wtx, etc.
          c (continue)
-----

          Berkeley DB special commands (located in db4/bin):

          db4/bin/db_dump ~/.bitcoin/wallets/wallet.dat
          db4/bin/db_dump -p ~/.bitcoin/wallets/wallet.dat | grep -C 2 0X1F

          db4/bin/db_load
-----

          C++ Compiler explorers to see compiler output and assembly:

          https://godbolt.org
          http://cpp.sh
-----

          To install the contents of bitcoind's bin subdirectory into the
          /usr/local/bin standard location for self-installed executables using
          the GNU coreutils install command:

          Using sudo:
          sudo install -m 0755 -o root -g root -t /usr/local/bin bitcoin-0.19.0/bin/*

          Using su:
          su -c 'install -m 0755 -o root -g root -t /usr/local/bin bitcoin-0.19.0/bin/*'
-----

          LogPrintf vs LogPrint:

          LogPrintf() logs the output into the debug.log file.

          LogPrint() only logs to the debug.log file
          (or to stdout if -printtoconsole has been enabled)
          when the corresponding -debug category is enabled.
-----

          To get and set bitcoind logging information, use:
          bitcoin-cli logging
          bitcoin-cli help logging

          To turn  on debug=net: bitcoin-cli logging '["net"]'
          To turn off debug=net: bitcoin-cli logging [] '["net"]'
          To turn  on debug=all: bitcoin-cli logging '["all"]'
          To turn off debug log: bitcoin-cli logging [] '["all"]'

          net: All messages related to communicating with other nodes on the
          network, including what P2P messages were sent and received and to
          whom and other information about the network messages.

          mempool: Messages related to actions done in the memory pool, most
          frequently transaction acceptance (`AcceptToMemoryPool`). Also
          includes transaction removals.

          validation: All messages related to the validation interface, most
          frequently `TransactionAddedToMempool` and `stored orphan tx`.

          http: Messages related to the HTTP server that is used to handle the
          RPC requests. These messages will typically be for the startup and
          shutdown of the server as well as received requests.

          tor: All messages related to using a TOR SOCKS5 proxy and TOR hidden
          service (used for receiving incoming connections over TOR). This
          includes messages about the creation and shutdown of the TOR hidden
          service and messages about the connection to the TOR proxy.

          bench: Messages about the benchmark performance of various parts of
          the software that can have performance issues.

          zmq: Messages about the ZeroMQ notification system, including the
          startup and shutdown of the service as well as when notifications are
          issued and new clients connected.

          walletdb (db in v.0.19 and earlier): Messages about the status of the
          Berkeley Database engine used for the wallet database, including
          messages about database flushes.

          rpc: Messages about the RPC server, including its startup and shutdown
          as well as when commands are issued.

          estimatefee: Messages about the fee estimation algorithm, including
          messages about when fee estimates are requested and information about
          the status of the fee estimator.

          addrman: Address Manager. Messages about the status of the address
          manager and when addresses are added or removed from the address
          manager database.

          selectcoins: Coin Selection. Messages about the UTXOs that are
          selected when sending money.

          reindex: Messages about the reindexing process, in particular errors
          about out-of-order blocks and repeated blocks.

          cmpctblock: Messages about the Compact Blocks relay protocol,
          including when blocks are partially downloaded or reconstructed.

          rand: Messages for when randomness is needed by any function.

          prune: Messages about local blockchain pruning, including the result
          of a pruning operation.

          proxy: Messages about using a SOCKS5 proxy and its authentication.

          mempoolrej: Messages about transactions rejected from the memory pool.

          libevent: Messages from the libevent library used for the HTTP server.

          coindb: Coin Database. Messages about the coin database which contains
          the UTXO set, including messages about database flushes and writes.

          qt: Messages about Qt, the GUI framework.

          leveldb: Messages about LevelDB which is used for the block indexes
          and coin database.
-----

          To build Bitcoin with Clang for better errors/use less resources,
          optionally with --enable-werror to highlight and halt on first error
          or CXXFLAGS flags like:

            -Wthread-safety
            -Wformat-security
            -Wthread-safety-analysis
            -Wrange-loop-analysis
            -Wredundant-decls
            -Wunused-variable
            -Wunused-parameter

          ./configure CXX=clang++ CC=clang CXXFLAGS=-Wthread-safety --enable-werror
-----

          Compact c-lightning debug log:
          lightning-cli getlog debug | jq '.log[] | .type + " " + .node_id + " " + .log' --raw-output | grep -v SKIPPED
-----

 #!/bin/bash
 shopt -s globstar
 for test_full_filename in **/*_tests.cpp; do
     test_name_file=`basename $test_full_filename .cpp`
     test_name_suite=`sed -n "s/^.*TEST_SUITE(\(.*_tests\).*$/\1/p" $test_full_filename`
     if [ $test_name_file != $test_name_suite ]; then
         echo "TestFilename: $test_name_file != TestSuitname: $test_name_suite"
     fi
 done
-----

gmaxwell: Avoid doing review that is inconsistent or focused on minutia or code
          style, especially if it comes across as oppressive rather than
          enabling.

          Ignore the smallest of nits, make style advice in a purely advisory
          way (as in, feel free to adjust if you happen to rebase, otherwise
          don't bother), move style adjustments -- if any -- until right before
          merging and not as soon as a PR opens. We've operated in that fashion
          before and I think the project accomplished a lot more at that time
          and was more enjoyable. (Likely not all due to coding style nits, but
          I would be surprised if it wasn't a factor.)

          I reiterate: the background level of refactors, style changes,
          cleanups, and other related activity is actively repelling multiple
          long term contributors, myself included. I beg: give it a bit of a
          rest, we have so many other things that are crying for our attention
          and our resolve. We should try to find some initiatives that we all
          feel more excited about, success with them would make it easier to
          work on other things... but right now a big style change is just not a
          unifying effort.
-----

sipa:     One easy way to make sure you have all the call sites is to rename it.

          Is test coverage missing for something? Break it, and see if tests fail :)

          If your end goal is integrating things into Bitcoin Core, getting
          familiar with the code is the best way to spend time.  To do that
          (which i very much encourage you to!) I think it's better to focus on
          one piece at a time and actually integrate it.

          Never put GitHub usernames in commits, nor PR descriptions which are
          automatically copied into merge commits.
-----

          Discussion on why to not use const with by value params:
          https://github.com/bitcoin/bitcoin/pull/19845#discussion_r489776345
-----

jnewbery  On insert vs emplace, in src/net_processing.cpp::ProcessMessage::L2836
          https://github.com/bitcoin/bitcoin/pull/19364#discussion_r448377825

          emplace wins when you have to construct a temporary in place which is
          then copied/moved to the container. When you already have the object
          in hand (as we do here), it makes no difference -- the object is just
          copied into the container.
-----

ryanofsky PR review comment at:
          https://github.com/bitcoin/bitcoin/pull/15849#pullrequestreview-231748721

          "I'm not sure the change from const std::string& to std::string&&
          is an improvement. You also now can't call SetInternalName with
          an lvalue (plain variable or reference). This is now an error:

          std::string name = "something";
          SetInternalName(name);

          "People get confused about this stuff, but if the goal is to just
          move-enable SetInternalName, it should take plain std::string,
          not const std::string&, and not std::string&&.

          "If a function just needs to read an argument (and not manipulate it
          before throwing it away or move it into a data structure) it should
          take const T&.

          "If a function wants to give callers the option of moving from an
          argument but still allow copies, it should take plain T.

          "If a function wants to force callers to move it should take T &&."
-----

practicalswift: Use `std::unique_ptr` (C++11) where possible.

          Rationale:
          1. Avoid resource leaks, e.g. forgetting to `delete` an object created using `new`
          2. Avoid undefined behaviour (specifically: double `delete`)

          From doc/developer-notes.md:

          - Use the RAII (Resource Acquisition Is Initialization) paradigm where
            possible. For example, by using `unique_ptr` for allocations in a
            function. Rationale: This avoids memory and resource leaks, and
            ensures exception safety.

          - Use `MakeUnique()` to construct objects owned by `unique_ptr`s.
            Rationale: `MakeUnique` is concise and ensures exception safety in
            complex expressions. `MakeUnique` is a temporary project-local
            implementation of `std::make_unique` (C++14).
-----

          On unique_ptr vs shared_ptr:

          promag: Why not use unique_ptr? I don't see sharing of message
          ownership.

          jonasschnelli: I think shared is fine here and does allow more
          flexible handling in future with little cost (ref counting).

          gmaxwell: Adding an extra memory allocation for every network message
          isn't free. I'm concerned that we may be falling into overusing
          sharedptr "just in case", which is bad because it leaks performance in
          a diffuse way that doesn't show up well in profiles. Please don't use
          sharedptr where unique will work. If something new comes in in the
          future that needs a sharedptr then we can switch then.

          sipa: unique_ptr and shared_ptr have the same number of allocations
          (if make_shared is used, at least), though shared_ptr uses more memory
          (both for the pointer itself and for the allocated memory), and has
          atomics to update the refcounts (which may reduce performance). To be
          clear, I agree we should avoid shared_ptr unless there is a good
          reason.

          jonasschnell: The reason why I used shared_ptr over unique_ptr is the
          polymorphism I'd like to use for the net messages and I once read that
          shared_ptr are recommended for polymorphic inheritance. However, I
          changed this PR now to use unique_ptr.
-----

harding   Idea: sendrawtransaction spends from cold wallet with network disabled
          to do a final inspection of the transaction in local mempool (mainly
          to check not forgetting an output and spending everything in fees).
          Before that try testmempoolaccept, and analyzepsbt before even
          broadcasting it.
-----

laanwj:   Release notes should refer to the RPC help for details instead of
          substituting for full documentation. Example:
          "Please refer to the RPC help of `getbalances` for details."
-----

Static fully valid pubkey in the Core codebase to reference in unit tests:

sipa       Use the generator
           0x0279BE667EF9DCBBAC55A06295CE870B07029BFCDB2DCE28D959F2815B16F81798
           with secret key 0x0000....000013, it's hardcoded in secp256k1

instagibbs functional test harness has static keys for quick reference when
           wallet isn't compiled

MarcoFalke ADDRESS_BCRT1_UNSPENDABLE or
           self.nodes[i].get_deterministic_priv_key().address
-----

gmaxwell   script to verify message signatures

           wget https://paste.debian.net/plain/1148565 && MSG=`cat 1148565 | head -n 7 | sed -e 's/\"//'`; echo $MSG ; tail -n 145 1148565 | while read -d $'\n' -a line ; do echo ${line[0]} `./bitcoin-cli verifymessage ${line[0]} ${line[1]} "$MSG"` ; done

-----

sjors      Tested it's reachable via IPv4 and IPv6 (and returns both types of
           addresses, and the filters work):

           dig -t A +trace seed.bitcoin.wiz.biz
           dig -t AAAA +trace seed.bitcoin.wiz.biz
           dig -6 -t A +trace seed.bitcoin.wiz.biz
           dig -6 -t AAAA +trace x5.seed.bitcoin.wiz.biz
-----

vasild     To view open network connections:
           sockstat -l

           For interactive communication with another host using the TELNET protocol:
           telnet 127.0.0.1 8333

-----

RPC command line curl:

curl --user `cat ~/.bitcoin/.cookie` --data-binary '{"jsonrpc": "1.0", "id": "curltest", "method": "getwalletinfo", "params": []}' -H 'content-type: text/plain;' http://127.0.0.1:8332/wallet/ | json_pp

-----

           To push to another user's remote branch:
           git push <remote> <your-branch>:<remote-branch>

           git push harding news125-pr20305:2020-11-25-newsletter
-----