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 consensus 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.

          We should be especially careful with testing on compiler updates,
          especially the Windows one. It's a low tier platform for GCC and time
          after time it's clear that it cannot be assumed even basic things keep
          working with a new version.
-----

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

          Better than git blame:
          git log -p `filename`

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

          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
          git log -L:CNodeStateStats:src/net_processing.h
          git log -L:getpeerinfo:src/rpc/net.cpp

          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 grep -E '^( |std::optional<)*std::pair.*\(' -- "*.h"

          git range-diff master PREV NEW

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

          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

          grep -cr encourage _site/en/newsletters | sort -r -n -t ":" -k 2 | head
-----

          Bitcoin Core daily coverage reports
          https://marcofalke.github.io/btc_cov/

-----
          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
-----

          Run tests with loopback address other than 127.0.0.1 for more parallelism
          This results in successfully running all of the tests that would
          normally be run, and without interfering with each other.  Ref:
          https://github.com/bitcoin/bitcoin/pull/26841#issuecomment-1376544220

ip netns add bitcoin_test_netns_1
ip netns exec bitcoin_test_netns_1 ip link set dev lo up
ip netns exec bitcoin_test_netns_1 ip link add dum0 type dummy
ip netns exec bitcoin_test_netns_1 ip addr add 10.1.1.1/32 dev dum0
ip netns exec bitcoin_test_netns_1 ip link set dum0 up
ip netns exec bitcoin_test_netns_1 sudo -u $(whoami) test/functional/test_runner.py -j 60

-----

raj       Generic steps for reviewing functional tests

          - Start python debugging and step through the *.py file.

          - At the start of a test run, the output prints the temp directory
            where regtest nodes are created. You can talk to them while you
            pause the python file, with bitcoin-cli -datadir=/tmp-filepath/node0/ <command>.

          - tail -f the debug.log and activate logging (net, rpc, etc.) to see
            communications happening in each nodes. Verify log is as expected.

          - If you want to print something from src/*.cpp, use LogPrintf() to
            print in the debug.log file, that you were already tailing.

          - Use python debug console to work with the python objects, call
            functions, check that object attributes are updated as expected.

          - Look for edge cases by changing test variables & observing failures.
-----

          Fuzz tests

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

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

          Compile with Clang:

          ./configure --enable-fuzz --with-sanitizers=fuzzer,address,undefined --disable-asm --enable-suppress-external-warnings CC=$(brew --prefix llvm)/bin/clang CXX=$(brew --prefix llvm)/bin/clang++

          May need to run `make clean` or `make distclean`.

          Run an individual fuzz test:
          FUZZ=process_message ./src/test/fuzz/fuzz

          Help: pass with -help=1 argument:
          FUZZ=process_message ./src/test/fuzz/fuzz -help=1

          vasild added fuzzing seeds to qa-assets by running the following:
          mkdir /tmp/fuzz
          FUZZ=netbase_dns_lookup ./src/test/fuzz/fuzz /tmp/fuzz
          ./test/fuzz/test_runner.py --m_dir /tmp/fuzz /path/to/bitcoin-core/qa-assets/fuzz_seed_corpus/


MarcoFalke jonatack: I couldn't run into a crash with 1kk iters in a single thread.
             - With "-use_value_profile=1 -workers=9 -jobs=9" and a folder to
               share the corpus, 400k iterations.
             - Same but without use_value_profile, 1.1kk iters for first crash.
             - Same as above (use_value_profile=1) w/o a shared corpus, 1.5kk.
             Result of this experiment (N=1):
             - Use a folder to share the corpus, Use more CPU, -use_value_profile=1

vasild    I run it like "src/test/fuzz/fuzz /some/new/empty/directory"
          You can supply a folder where to put the seeds:
          FUZZ=node_eviction src/test/fuzz/fuzz ../qa-assets/fuzz_seed_corpus/node_eviction
          FUZZ=addrman src/test/fuzz/fuzz ../qa-assets/fuzz_seed_corpus/addrman

          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

practicalswift fuzz survival test
               https://gist.github.com/practicalswift/e3de0d96af31f42d3a2f5bcb6f631432
-----

          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.
-----

          Run the CI locally:

          MAKEJOBS="-j5" FILE_ENV="./ci/test/00_setup_env_win64.sh" ./ci/test_run_all.sh

          32-bit depends build:

          CONFIG_SITE="$PWD/depends/i686-pc-linux-gnu/share/config.site" ./configure CC="clang -m32" CXX="clang++ -m32"
          make V=1

          CONFIG_SITE="$PWD/depends/i686-pc-linux-gnu/share/config.site" ./configure --enable-suppress-external-warnings
          make -j9 > /dev/null
-----

          Bench:

          Online docs: https://nanobench.ankerl.com/tutorial.html

martinus  It is really helpful to run a benchmark in an endless loop like so:

          NANOBENCH_ENDLESS=BlockToJsonVerbose ./src/bench/bench_bitcoin -filter=BlockToJsonVerbose

          and then fire up a profiler like `perf top` or hotspot
          (https://github.com/KDAB/hotspot) to see where all the cycles are used.

          ./src/bench/bench_bitcoin
          ./src/bench/bench_bitcoin -?
          ./src/bench/bench_bitcoin -h
          ./src/bench/bench_bitcoin -filter='EvictionProtection*.*'
-----

          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=2020-01-01 --until=2020-12-31 --no-merges | wc -l

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

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

          List top 20 contributors in commits per year
          for i in 2012 2013 2014 2015 2016 2017 2018 2019 2020 2021; 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)

          n (run) or r –> executes the program from start to end.
          break or b –> sets breakpoint on a particular line.
          disable -> disable a breakpoint.
          enable –> enable a disabled breakpoint.
          next or n -> executes next line of code, but don’t dive into functions.
          step –> go to next instruction, diving into the function.
          list or l –> displays the code.
          print or p –> used to display the stored value.
          quit or q –> exits out of gdb.
          clear –> to clear all breakpoints.
          continue or c –> continue normal execution.

vasild    that should boil down to something like "gdb ./src/bitcoind", then
          inside gdb, press "r" to start it, as soon as it crashes type "bt"
          to get the backtrace, type "up" or "f 1", "p contents.size_",
          "p contents.data_", "p n", and "p data"
-----
          Multinode / Multiwallet Bitcoin regtest network
          https://github.com/FreekPaans/bitcoin-multi-node-regtest
-----
          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"]'

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

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

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

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

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

          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.

          i2p: All messages related to using the I2P privacy network

          ipc:

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

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

          lock: All lock contentions and their duration.

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

          mempoolrej: Messages about transactions rejected from the memory pool.

          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.

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

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

          qt: Messages about Qt, the GUI framework.

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

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

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

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

          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.

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

          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.

          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.

-----

ryanofsky How to use a LOCK_RETURNED helper annotation to be able to keep using
          clang thread safety lock annotations when an object (here m_mempool)
          is null:

          RecursiveMutex* MempoolMutex() const LOCK_RETURNED(m_mempool->cs)
          {
              return m_mempool ? &m_mempool->cs : nullptr;
          }

          Described in:
          https://github.com/bitcoin/bitcoin/pull/22415#issuecomment-875999127
-----

          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 news152-review-club:2021-06-09-newsletter
-----

satoshi    https://www.metzdowd.com/pipermail/cryptography/2008-November/014832.html

"I actually did this kind of backwards.  I had to write all the code before I
could convince myself that I could solve every problem, then I wrote the paper.
I think I will be able to release the code sooner than I could write a detailed
spec.  You're already right about most of your assumptions where you filled in
the blanks."
-----

Run this to print the bitcoin white paper from the blockchain:

bitcoin-cli getblock 00000000000000ecbbff6bafb7efa2f7df05b227d5c73dca8f2635af32a2e949 0 | tail -c+92167 | for ((o=0;o<946;++o)) ; do read -rN420 x ; echo -n ${x::130}${x:132:130}${x:264:130} ; done | xxd -r -p | tail -c+9 | head -c184292 > bitcoin.pdf

bitcoin-cli getrawtransaction 54e48e5f5c656b26c3bca14a8c95aa583d07ebe84dde3b7dd4a78f4e4186e713 | sed 's/0100000000000000/\n/g' | tail -n +2 | cut -c7-136,139-268,271-400 | tr -d '\n' | cut -c17-368600 | xxd -p -r > bitcoin.pdf
-----

Why onlynet=onion is not a particularly recommended option

sipa:

It's a trade-off. Only connecting out over Tor is more private of course when
you combine it with no reachable IPv4/IPv6 address, in particular if you want to
broadcast transactions without them being correlatable with your IP.

On the other hand, if you only make random Tor connections, you're much more
vulnerable to Sybil attacks (as Tor addresses have 0 cost to create, an attacker
can just flood the network with 1000s of apparent Tor Bitcoin nodes, and have a
high probability of receiving all outbound Tor connections a Tor-only node
makes). This is significantly less a concern with IPv4/IPv6 (especially with
asmap) due to cost of getting IPs in many networks. It's also less a concern if
you have -addnoded connections to trusted peers (even if they're onion
addresses).

me:

For example, search on "onlynet" in
https://en.bitcoin.it/wiki/Setting_up_a_Tor_hidden_service

"not particularly recommended...if everyone used onlynet=onion nobody on the
onion bitcoin chain would be able to communicate with the clearnet chain. It is
essential that some nodes access both clearnet and Tor...If you only wish to
give access to your node to other Tor users, do not use it. "

-----

           How to see your version of Boost:

           dpkg -s libboost-dev | grep 'Version'
           echo -e '#include <boost/version.hpp>\nBOOST_VERSION' | gcc -x c++ -E -
           dpkg -S /usr/include/boost/version.hpp
-----

template<typename Function>
void printtime_inplace(string title, Function func)
{
    //...
    func(title);
    //...
}

-----

           When should static_cast, dynamic_cast, const_cast and reinterpret_cast be used?
           https://stackoverflow.com/questions/332030/when-should-static-cast-dynamic-cast-const-cast-and-reinterpret-cast-be-used

           https://github.com/bitcoin/bitcoin/pull/20965#issuecomment-770375623

           Q: static_casts: What is an example of a compile time check that is
           enabled when using those for numeric types?

           A: If the destination type is numeric, the named cast asserts at
           compile time that also the source type is numeric, while a c-style
           cast doesn't in all cases:

           long foo = (long)some_pointer; // compiles happily
           long bar = static_cast<long>some_pointer; // "error: invalid ‘static_cast’ from type ‘char*’ to type ‘long int’"

           Also note that modern C++ coding guidelines discourage the use of C casts:
           https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Res-casts-named
           https://google.github.io/styleguide/cppguide.html#Casting
-----

           Brace (uniform) initialization

           https://github.com/bitcoin/bitcoin/pull/20685#discussion_r584631345

           Brace (uniform) initialization raises compile time warnings (e.g. for
           narrowing and conversions), and named casts provide compile time
           checks as well.

           https://google.github.io/styleguide/cppguide.html#Casting
           https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Res-casts-named

           "Use brace initialization to convert arithmetic types
           (e.g. int64{x}). This is the safest approach because code will not
           compile if conversion can result in information loss. The syntax is
           also concise. Use static_cast as the equivalent of a C-style cast
           that does value conversion."

           "Brace initialization makes it clear that the type conversion was
           intended and also prevents conversions between types that might
           result in loss of precision. (It is a compilation error to try to
           initialize a float from a double in this fashion, for example.)"


           https://github.com/bitcoin/bitcoin/pull/21848#issuecomment-831763809

           Important note which may not be immediately clear for all reviewers:

           int32_t i{j}; is not necessarily equivalent to int32_t i = j; or int32_t i(j);.

           Thus the choice of {}-initialization is not a cosmetic style choice.

           int32_t i{j}; is the preferred choice not because it looks nicer, but because it is safer.

           Safer how? Live demo:

           $ cling
           [cling]$ #include <cstdint>
           [cling]$ int64_t j = 2147483648
           (long) 2147483648
           [cling]$ int32_t non_uniform_1 = j
           (int) -2147483648
           [cling]$ int32_t non_uniform_2(j)
           (int) -2147483648
           [cling]$ int32_t uniform{j}

           input_line_9:2:18: error: non-constant-expression cannot be narrowed
           from type 'int64_t' (aka 'long') to 'int32_t' (aka 'int') in initializer list

           Note the silent narrowing from 2147483648 to -2147483648 for = j and
           (j), but not for {j}.  That is why the {}-initializer syntax is said
           to be safer than the other forms of initializations: by using {}
           we're guaranteed not to be hit by an unexpected narrowing conversion.

           See C++ Core Guidelines: ES.23: Prefer the {}-initializer syntax.
           https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#es23-prefer-the--initializer-syntax
-----

jonatack   https://github.com/bitcoin/bitcoin/pull/20210#discussion_r528835313

           -> In this case, we can remove the default argument in the constructor declaration

           -> As to your last point, it suggests the default member
              `m_inbound_onion` initializer not set a value, since we are doing
              it dynamically in the ctor. It's true that I see a build error in
              this case, but not without the change in `net.h:1099`

           More generally, from what I can gather, the ideas behind in-class
           default member initializers added in C++11 were (a) to allow a
           non-static data member to be initialized where it is declared,
           e.g. in its class. A constructor can then use the initializer when
           run-time initialization is needed, which can then (b) tidy up the
           code up by avoiding doing it in the member initializer list of the
           constructor. This provides extra benefits in classes with multiple
           constructors.

           I guess one can see the default member initializer as the general
           default, particularly useful for constant initializers, that can be
           overridden by any specific ones in member initializer lists in the
           constructors.

           Some resources:
           - https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines.html#Rc-in-class-initializer
           - https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines.html#c49-prefer-initialization-to-assignment-in-constructors
           - https://en.cppreference.com/w/cpp/language/data_members#member-initialization
           - https://www.stroustrup.com/C++11FAQ.html#member-init
-----

           How to deterministically reproduce a rare test issue using rr
           https://github.com/bitcoin/bitcoin/issues/16337#issuecomment-629360810
           https://github.com/bitcoin/bitcoin/issues/16337#issuecomment-629041023

$ git clone https://github.com/bitcoin/bitcoin
$ cd bitcoin/
$ ./autogen.sh
$ ./configure --with-incompatible-bdb
$ make
$ git clone https://github.com/mozilla/rr
$ cd rr/
$ ./configure
$ make
$ cd ../
$ rm -rf test_bitcoin_chaos
$ while rr/bin/rr record --chaos -o test_bitcoin_chaos src/test/test_bitcoin -l test_suite -t blockfilter_index_tests -- DEBUG_LOG_OUT; do
      rm -rf test_bitcoin_chaos
  done
-----

While here, could you add the missing brackets to the conditional per the developer notes, or hoist `return false;` to the same line?

We require brackets with conditionals over one line due to CVE-2014-1266 (the Apple "goto fail" vulnerability, see also https://dwheeler.com/essays/apple-goto-fail).

           Why we require brackets with conditionals over one line:
           CVE-2014-1266: the Apple "goto fail" vulnerability
           https://dwheeler.com/essays/apple-goto-fail
-----

           A [[nodiscard]] attribute is most useful in two cases:

           - if a function has no effects beyond returning a certain result,
             i.e. is pure. If the result is not used, the call is certainly
             useless. On the other hand, discarding the result would not be
             incorrect.

           - if the return value must be checked, e.g. for a C-like interface
             that returns error codes instead of throwing. This is the primary
             use case. For idiomatic C++, that's going to be quite rare.
-----

           export TMPDIR=/dev/shm

           mkdir /dev/shm/fuzz_temp_seeds
           export TMPDIR=/dev/shm ; time src/test/fuzz/utxo_total_supply /dev/shm/fuzz_temp_seeds
-----

#!/usr/bin/env python3
import subprocess
with open('contrib/seeds/nodes_test.txt') as f:
    for line in (line for line in (line.rstrip().split('#', 1)[0] for line in f) if line):
        subprocess.call(["nc", "-v", "-x", "127.0.0.1:9050", "-z"] + line.split(':'))
-----

alex bosworth

Threats to Lightning Network:
1. Miner censorship prevents predictable confirmation
2. Free message passing used as denial of service
3. Funding and interest dry up before critical mass
4. Liquidity fees fails to represent real market
5. Capital requirements too high in practice

-----

Guix       https://gist.github.com/hebasto/7293726cbfcd0b58e1cfd5418316cee3
-----

key server https://keys.openpgp.org/search?q=jon%40atack.com
-----

           constexpr vs macros (#define)
           https://stackoverflow.com/questions/42388077/constexpr-vs-macros/42388687
-----

           Bitcoind dashboards
           https://bitcoind.observer (b10c)
           https://bitcoinperf.com (jamesob)
-----