Page MenuHomePhabricator

Add proof-of-concept for submitblocksolution and block template manager
Needs RevisionPublic

Authored by jasonbcox on Aug 5 2020, 21:41.

Details

Reviewers
deadalnix
Group Reviewers
Restricted Project
Summary

This proof-of-concept is based on my old proposal for a replacement to getblocktemplate: https://docs.google.com/document/d/10YVLdltuppEoizAVoovPTOZbqthbsa2t0MXHhaDo4bI/edit

Quick summary of the rationale:
getblocktemplate passes the entire block (including all transactions) to the mining software and
when a block is solved, the mining software passes that entire block back to the node via submitblock.
This cross-talk is unnecessary since the node typically dictates the contents and ordering of transactions.
As block sizes increase, this becomes a scaling bottleneck and actually poses risks to miners by delaying
block validation and propagation.

This patch introduces a working portion of submitblocksolution along with introducing a bare-bones
BlockTemplateManager class. While there remains a lot of work to be done, this patch gives a hint to
how mining with the new templating system may be done in the future:

  1. Miners call (a not yet implemented version of) getblocktemplate and receive a template ID along with some other fields. Among these fields is a merkle proof so that the merkleroot maybe determined.
  2. When a solution is found, the mining software passes the template ID and some modified fields back to the node software via submitblocksolution.

This process is intended to be similar to current mining processes in order for integration to be easy,
though some changes to the mining software will be necessary. You'll note in the tests that the current
getblocktemplate is still used in step 1 described above, demonstrating how similar they are.

The BlockTemplateManager class leaves even more to be desired. It currently stores nothing but the most
recent block template for testing purposes. In the future, it should be able to:

  • Track template IDs
  • Store multiple block templates
  • Maintain the memory for those templates in a sane way, ideally through some mechanism for snapshotting the mempool

The API is not fully fleshed out. The current iteration was chosen as a compromise between where the API design is
heading and maintaining compatibility with the current getblocktemplate implementation.

Test Plan
ninja check check-functional

Diff Detail

Repository
rABC Bitcoin ABC
Branch
submitblocksolution
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 12317
Build 24830: Build Diffbuild-clang-10 · build-diff · build-without-wallet · build-clang-tidy
Build 24829: arc lint + arc unit

Event Timeline

Snippet of first build failure:

[297/472] Building CXX object src/CMakeFiles/server.dir/index/txindex.cpp.o
[298/472] Building CXX object src/CMakeFiles/server.dir/validationinterface.cpp.o
[299/472] Building CXX object src/CMakeFiles/server.dir/versionbits.cpp.o
[300/472] Building CXX object src/CMakeFiles/server.dir/noui.cpp.o
[301/472] Building CXX object src/CMakeFiles/bitcoin-tx.dir/bitcoin-tx.cpp.o
[302/472] Building C object src/secp256k1/CMakeFiles/ecmult-bench.dir/src/bench_ecmult.c.o
[303/472] Building C object src/secp256k1/CMakeFiles/recover-bench.dir/src/bench_recover.c.o
[304/472] Building C object src/secp256k1/CMakeFiles/sign-bench.dir/src/bench_sign.c.o
[305/472] Building C object src/secp256k1/CMakeFiles/secp256k1.dir/src/secp256k1.c.o
[306/472] Linking C static library src/secp256k1/libsecp256k1.a
[307/472] Building CXX object src/CMakeFiles/server.dir/interfaces/chain.cpp.o
[308/472] Building CXX object src/CMakeFiles/server.dir/rpc/abc.cpp.o
[309/472] Building CXX object src/CMakeFiles/server.dir/timedata.cpp.o
[310/472] Linking C executable src/secp256k1/ecmult-bench
[311/472] Building C object src/secp256k1/CMakeFiles/verify-bench.dir/src/bench_verify.c.o
[312/472] Linking CXX static library src/libcommon.a
[313/472] Linking C executable src/secp256k1/recover-bench
[314/472] Linking C executable src/secp256k1/sign-bench
[315/472] Linking CXX static library src/libscript.a
[316/472] Linking C executable src/secp256k1/verify-bench
[317/472] Building CXX object src/test/CMakeFiles/testutil.dir/util/blockfilter.cpp.o
[318/472] Linking CXX static library src/libbitcoinconsensus.a
[319/472] Building CXX object src/test/CMakeFiles/testutil.dir/util/logging.cpp.o
[320/472] Building C object src/secp256k1/CMakeFiles/internal-bench.dir/src/bench_internal.c.o
[321/472] Linking CXX executable src/bitcoin-cli
[322/472] Linking CXX shared library src/libbitcoinconsensus.so.0.0.0
[323/472] Linking C executable src/secp256k1/internal-bench
[324/472] Creating library symlink src/libbitcoinconsensus.so.0 src/libbitcoinconsensus.so
[325/472] Building CXX object src/test/CMakeFiles/testutil.dir/util/str.cpp.o
[326/472] Building CXX object src/test/CMakeFiles/testutil.dir/util/transaction_utils.cpp.o
[327/472] Linking CXX executable src/bitcoin-tx
[328/472] Building CXX object src/CMakeFiles/server.dir/interfaces/node.cpp.o
[329/472] Building CXX object src/wallet/CMakeFiles/wallet.dir/coinselection.cpp.o
[330/472] Building CXX object src/wallet/CMakeFiles/wallet.dir/crypter.cpp.o
[331/472] Building CXX object src/wallet/CMakeFiles/wallet.dir/db.cpp.o
[332/472] Building CXX object src/CMakeFiles/server.dir/rpc/mining.cpp.o
FAILED: src/CMakeFiles/server.dir/rpc/mining.cpp.o 
/usr/bin/ccache /usr/bin/clang++-10  -DBOOST_AC_USE_STD_ATOMIC -DBOOST_SP_USE_STD_ATOMIC -DBUILD_BITCOIN_INTERNAL -DENABLE_AVX2 -DENABLE_SHANI -DENABLE_SSE41 -DHAVE_BUILD_INFO -DHAVE_CONFIG_H -DHAVE_CONSENSUS_LIB -DLEVELDB_ATOMIC_PRESENT -DLEVELDB_PLATFORM_POSIX -DOS_LINUX -I../../src/leveldb/helpers/memenv -I../../src/. -Isrc -I../../src/univalue/include -Isrc/crypto/.. -I../../src/secp256k1/include -I../../src/leveldb/include -isystem /usr/include/jemalloc -isystem /usr/include/miniupnpc -Werror -g -O2 -fPIC -fvisibility=hidden   -fstack-protector-all -Wstack-protector -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 -Wall -Wextra -Wformat -Wvla -Wcast-align -Wunused-parameter -Wmissing-braces -Wthread-safety-analysis -Wshadow -Wshadow-field -Wrange-loop-analysis -Wredundant-decls -Wformat-security -Wredundant-move -Wno-unused-parameter -Wno-implicit-fallthrough -pthread -std=gnu++14 -MD -MT src/CMakeFiles/server.dir/rpc/mining.cpp.o -MF src/CMakeFiles/server.dir/rpc/mining.cpp.o.d -o src/CMakeFiles/server.dir/rpc/mining.cpp.o -c ../../src/rpc/mining.cpp
../../src/rpc/mining.cpp:603:26: error: moving a temporary object prevents copy elision [-Werror,-Wpessimizing-move]
        pblocktemplate = std::move(
                         ^
../../src/rpc/mining.cpp:603:26: note: remove std::move call here
        pblocktemplate = std::move(
                         ^~~~~~~~~~
1 error generated.
[333/472] Building CXX object src/CMakeFiles/server.dir/miner.cpp.o
[334/472] Building CXX object src/CMakeFiles/server.dir/txdb.cpp.o
[335/472] Building CXX object src/CMakeFiles/server.dir/wallet/init.cpp.o
[336/472] Building CXX object src/wallet/CMakeFiles/wallet.dir/coincontrol.cpp.o
[337/472] Building CXX object src/CMakeFiles/server.dir/net.cpp.o
[338/472] Building CXX object src/wallet/CMakeFiles/wallet.dir/fees.cpp.o
[339/472] Building CXX object src/wallet/CMakeFiles/wallet-tool.dir/wallettool.cpp.o
[340/472] Building CXX object src/test/CMakeFiles/testutil.dir/util/setup_common.cpp.o
[341/472] Building CXX object src/CMakeFiles/server.dir/init.cpp.o
[342/472] Building CXX object src/CMakeFiles/server.dir/ui_interface.cpp.o
[343/472] Building CXX object src/CMakeFiles/server.dir/rpc/rawtransaction.cpp.o
[344/472] Building CXX object src/wallet/CMakeFiles/wallet.dir/__/interfaces/wallet.cpp.o
[345/472] Building CXX object src/CMakeFiles/server.dir/validation.cpp.o
ninja: build stopped: subcommand failed.
Build build-clang-10 failed with exit code 1

Snippet of first build failure:

rpc_deriveaddresses.py --usecli         | ✓ Passed  | 1 s
rpc_estimatefee.py                      | ✓ Passed  | 1 s
rpc_getblockfilter.py                   | ✓ Passed  | 1 s
rpc_getblockstats.py                    | ✓ Passed  | 1 s
rpc_getchaintips.py                     | ✓ Passed  | 2 s
rpc_help.py                             | ✓ Passed  | 1 s
rpc_invalidateblock.py                  | ✓ Passed  | 6 s
rpc_misc.py                             | ✓ Passed  | 1 s
rpc_named_arguments.py                  | ✓ Passed  | 1 s
rpc_net.py                              | ✓ Passed  | 1 s
rpc_preciousblock.py                    | ✓ Passed  | 1 s
rpc_psbt.py                             | ✓ Passed  | 23 s
rpc_scantxoutset.py                     | ✓ Passed  | 5 s
rpc_setban.py                           | ✓ Passed  | 2 s
rpc_signmessage.py                      | ✓ Passed  | 1 s
rpc_signrawtransaction.py               | ✓ Passed  | 1 s
rpc_txoutproof.py                       | ✓ Passed  | 2 s
rpc_uptime.py                           | ✓ Passed  | 1 s
rpc_users.py                            | ✓ Passed  | 2 s
rpc_whitelist.py                        | ✓ Passed  | 1 s
tool_wallet.py                          | ✓ Passed  | 3 s
wallet_abandonconflict.py               | ✓ Passed  | 12 s
wallet_address_types.py                 | ✓ Passed  | 19 s
wallet_avoidreuse.py                    | ✓ Passed  | 2 s
wallet_backup.py                        | ✓ Passed  | 41 s
wallet_balance.py                       | ✓ Passed  | 23 s
wallet_basic.py                         | ✓ Passed  | 30 s
wallet_create_tx.py                     | ✓ Passed  | 6 s
wallet_createwallet.py                  | ✓ Passed  | 2 s
wallet_createwallet.py --usecli         | ✓ Passed  | 3 s
wallet_disable.py                       | ✓ Passed  | 1 s
wallet_dump.py                          | ✓ Passed  | 3 s
wallet_encryption.py                    | ✓ Passed  | 6 s
wallet_groups.py                        | ✓ Passed  | 10 s
wallet_hd.py                            | ✓ Passed  | 5 s
wallet_import_rescan.py                 | ✓ Passed  | 4 s
wallet_import_with_label.py             | ✓ Passed  | 1 s
wallet_importmulti.py                   | ✓ Passed  | 3 s
wallet_importprunedfunds.py             | ✓ Passed  | 1 s
wallet_keypool.py                       | ✓ Passed  | 3 s
wallet_keypool_topup.py                 | ✓ Passed  | 2 s
wallet_labels.py                        | ✓ Passed  | 1 s
wallet_listreceivedby.py                | ✓ Passed  | 22 s
wallet_listsinceblock.py                | ✓ Passed  | 4 s
wallet_listtransactions.py              | ✓ Passed  | 18 s
wallet_multiwallet.py                   | ✓ Passed  | 45 s
wallet_multiwallet.py --usecli          | ✓ Passed  | 20 s
wallet_reorgsrestore.py                 | ✓ Passed  | 3 s
wallet_resendwallettransactions.py      | ✓ Passed  | 9 s
wallet_txn_clone.py                     | ✓ Passed  | 1 s
wallet_txn_clone.py --mineblock         | ✓ Passed  | 3 s
wallet_txn_doublespend.py               | ✓ Passed  | 1 s
wallet_txn_doublespend.py --mineblock   | ✓ Passed  | 3 s
wallet_zapwallettxes.py                 | ✓ Passed  | 4 s

ALL                                     | ✓ Passed  | 757 s (accumulated) 
Runtime: 152 s

ninja: build stopped: cannot make progress due to previous errors.
Build build-diff failed with exit code 1
deadalnix requested changes to this revision.Aug 8 2020, 12:55
deadalnix added a subscriber: deadalnix.

I don't think this design makes a lot of sense.

The ids will be node local, so you can't use that system to submit the template to several nodes, the API is all screwed up (the coinbase is part of the block header now ?), doesn't work - there is no way to propagate the block from there. There is also no attempt to look at existing API that achieve something similar and conform to them.

I'd suggest to just ditch this and start build each block in a way that make sense. There needs to be a block template manager. It can build template, expire them by time and so on. There is ton to do there and that's one track you can move forward on. Then you need some API to interact with this. You might not want to exposes these API the way they are in the doc when they do not work. But if the other things are done well, the API shouldn't be anything more than a thin wrapper.

src/rpc/mining.cpp
811

It makes no sense that the coinbase is part of the header.

This revision now requires changes to proceed.Aug 8 2020, 12:55