a60f863d3e scripted-diff: Replace GenTxidVariant with GenTxid (marcofleon)
c8ba199598 Remove old GenTxid class (marcofleon)
072a198ea4 Convert remaining instances of GenTxid to GenTxidVariant (marcofleon)
1b528391c7 Convert `txrequest` to GenTxidVariant (marcofleon)
bde4579b07 Convert `txdownloadman_impl` to GenTxidVariant (marcofleon)
c876a892ec Replace GenTxid with Txid/Wtxid overloads in `txmempool` (marcofleon)
de858ce2be move-only: make GetInfo a private CTxMemPool member (stickies-v)
eee473d9f3 Convert `CompareInvMempoolOrder` to GenTxidVariant (marcofleon)
243553d590 refactor: replace get_iter_from_wtxid with GetIter(const Wtxid&) (stickies-v)
fcf92fd640 refactor: make CTxMemPool::GetIter strongly typed (marcofleon)
11d28f21bb Implement GenTxid as a variant (marcofleon)
Pull request description:
Part of the [type safety refactor](https://github.com/bitcoin/bitcoin/pull/32189).
This PR changes the GenTxid class to a variant, which holds both Txids and Wtxids. This provides compile-time type safety and eliminates the manual type check (bool m_is_wtxid). Variables that can be either a Txid or a Wtxid are now using the new GenTxid variant, instead of uint256.
ACKs for top commit:
w0xlt:
ACK a60f863d3e
dergoegge:
Code review ACK a60f863d3e
maflcko:
review ACK a60f863d3e🎽
theStack:
Code-review ACK a60f863d3e
Tree-SHA512: da9b73b7bdffee2eb9281a409205519ac330d3336094d17681896703fbca8099608782c9c85801e388e4d90af5af8abf1f34931f57bbbe6e9674d802d6066047
a18e572328 test: more template verification tests (Sjors Provoost)
10c908808f test: move gbt proposal mode tests to new file (Sjors Provoost)
94959b8dee Add checkBlock to Mining interface (Sjors Provoost)
6077157531 ipc: drop BlockValidationState special handling (Sjors Provoost)
74690f4ed8 validation: refactor TestBlockValidity (Sjors Provoost)
Pull request description:
This PR adds the IPC equivalent of the `getblocktemplate` RPC in `proposal` mode.
In order to do so it has `TestBlockValidity` return error reasons as a string instead of `BlockValidationState`. This avoids complexity in IPC code for handling the latter struct.
The new Mining interface method is used in `miner_tests`.
It's not used by the `getblocktemplate` and `generateblock` RPC calls, see https://github.com/bitcoin/bitcoin/pull/31981#discussion_r2096473337
The `inconclusive-not-best-prevblk` check is moved from RPC
code to `TestBlockValidity`.
Test coverage is increased by `mining_template_verification.py`.
Superseedes #31564
## Background
### Verifying block templates (no PoW)
Stratum v2 allows miners to generate their own block template. Pools may wish (or need) to verify these templates. This typically involves comparing mempools, asking miners to providing missing transactions and then reconstructing the proposed block.[^0] This is not sufficient to ensure a proposed block is actually valid. In some schemes miners could take advantage of incomplete validation[^1].
The Stratum Reference Implementation (SRI), currently the only Stratum v2 implementation, collects all missing mempool transactions, but does not yet fully verify the block.[^2]. It could use the `getblocktemplate` RPC in `proposal` mode, but using IPC is more performant, as it avoids serialising up to 4 MB of transaction data as JSON.
(although SRI could use this PR, the Template Provider role doesn't need it, so this is _not_ part of #31098)
[^0]: https://github.com/stratum-mining/sv2-spec/blob/main/06-Job-Declaration-Protocol.md
[^1]: https://delvingbitcoin.org/t/pplns-with-job-declaration/1099/45?u=sjors
[^2]: https://github.com/stratum-mining/stratum/blob/v1.1.0/roles/jd-server/src/lib/job_declarator/message_handler.rs#L196
ACKs for top commit:
davidgumberg:
reACK a18e572328
achow101:
ACK a18e572328
TheCharlatan:
ACK a18e572328
ryanofsky:
Code review ACK a18e572328 just adding another NONFATAL_UNREACHABLE since last review
Tree-SHA512: 1a6c29f45a1666114f10f55aed155980b90104db27761c78aada4727ce3129e6ae7a522d90a56314bd767bd7944dfa46e85fb9f714370fc83e6a585be7b044f1
029ba1a21d index: remove CBlockIndex access from CustomAppend() (furszy)
91b7ab6c69 refactor: index, simplify CopyHeightIndexToHashIndex to process single block (furszy)
6f1392cc42 indexes, refactor: Remove remaining CBlockIndex* uses in index Rewind methods (Ryan Ofsky)
0a248708dc indexes, refactor: Stop requiring CBlockIndex type to call IsBIP30Unspendable (Ryan Ofsky)
331a25cb16 test: indexes, avoid creating threads when sync runs synchronously (furszy)
Pull request description:
Combining common refactors from #24230 and #26966, aiming to move both efforts forward while reducing their size and review burden.
Broadly, #24230 focuses on enabling indexes to run in a separate process, and #26966 aims to parallelize the indexes initial synchronization process. A shared prerequisite for both is ensuring that only the base index class interacts with the node’s chain internals - child index classes should instead operate solely through chain events.
This PR moves disk read lookups from child index classes to the base index class. It also includes a few documentation improvements and a test-only code cleanup.
ACKs for top commit:
maflcko:
review ACK 029ba1a21d👡
achow101:
ACK 029ba1a21d
TheCharlatan:
Re-ACK 029ba1a21d
davidgumberg:
ACK 029ba1a21d
mzumsande:
Code Review ACK 029ba1a21d
Tree-SHA512: f073af407fc86f228cb47a32c7bcf2241551cc89ff32059317eb81d5b86fd5fda35f228d2567e0aedbc9fd6826291f5fee05619db35ba44108421ae04d11e6fb
Moved CBlockUndo disk read lookups from child index classes to
the base index class.
The goal is for child index classes to synchronize only through
events, without directly accessing the chain database.
This change will enable future parallel synchronization mechanisms,
reduce database access (when batched), and contribute toward the
goal of running indexes in a separate process (with no chain
database access).
Besides that, this commit also documents how NextSyncBlock() behaves.
It is not immediately clear this function could return the first
block after the fork point during a reorg.
Move ReadBlock code from CoinStatsIndex::CustomRewind to BaseIndex::Rewind
Move ReadUndo code from CoinStatsIndex::ReverseBlock to BaseIndex::Rewind
This commit does change behavior slightly. Since the new CustomRemove
methods only take a single block at a time instead of a range of
disconnected blocks, when they call CopyHeightIndexToHashIndex they will
now do an index seek for each removed block instead of only seeking once
to the height of the earliest removed block. Seeking instead of scanning
is a little worse for performance if there is a >1 block reorg, but
probably not noticeable unless the reorg is very deep.
This can be reproduced according to the developer notes with something
like
( cd ./src/ && ../contrib/devtools/run-clang-tidy.py -p ../bld-cmake -fix -j $(nproc) )
Also, the header related changes were done manually.
e8661aac75 wallet: drop watch-only things from interface (Sjors Provoost)
e99188e7da qt: drop unused watch-only functionality (Sjors Provoost)
Pull request description:
The watch-only functionality in the GUI was only used for legacy wallets. Watch-only descriptor wallets do not use this.
The only visible changes of this PR should be:
- dropped "Spendable:" label from the overview tab
- column width cache is reset
This PR also removes some unused variables from the interface.
ACKs for top commit:
davidgumberg:
Review ACK e8661aac75.
hebasto:
ACK e8661aac75, I have reviewed the code and it looks OK. The `src/qt/forms/overviewpage.ui` form was reviewed in Qt Designer.
Tree-SHA512: d7edb0f167e0b934075398a76eddca69890bb36848a918c932b1c2cea85ee87285e876cbfdf1f6dec7adf26b9f405fb558c70bec0c84585c0a9df33c2af78393
ee045b61ef rpc, psbt: Require sighashes match for descriptorprocesspsbt (Ava Chow)
2b7682c372 psbt: use sighash type field to determine whether to remove non-witness utxos (Ava Chow)
28781b5f06 psbt: Add sighash types to PSBT when not DEFAULT or ALL (Ava Chow)
15ce1bd73f psbt: Enforce sighash type of signatures matches psbt (Ava Chow)
1f71cd337a wallet: Remove sighash type enforcement from FillPSBT (Ava Chow)
4c7d767e49 psbt: Check sighash types in SignPSBTInput and take sighash as optional (Ava Chow)
a118256948 script: Add IsPayToTaproot() (Ava Chow)
d6001dcd4a wallet: change FillPSBT to take sighash as optional (Ava Chow)
e58b680923 psbt: Return PSBTError from SignPSBTInput (Ava Chow)
2adfd81532 tests: Test PSBT sighash type mismatch (Ava Chow)
5a5d26d612 psbt: Require ECDSA signatures to be validly encoded (Ava Chow)
Pull request description:
Currently, we do not add the sighash field to PSBTs at all, even when we have signed with a non-default sighash. This PR changes the behavior such that when we (attempt to) sign with a sighash other than DEFAULT or ALL, the sighash type field will be added to the PSBT to inform the later signers that a different sighash type was used by a signer. Notably, this is necessary for MuSig2 support as all signers must sign using the same sighash type, but the sighash is not provided in partial signatures.
Furthermore, because the sighash type can also be provided on the command line, we require that if both a command line sighash type and the sighash field is present, they must specify the same sighash type. However, this was being checked by the wallet, rather than the signing code, so the `descriptorprocesspsbt` RPC was not enforcing this restriction at all, and in fact ignored the sighash field entirely. This PR refactors the checking code so that the underlying PSBT signing function `SignPSBTInput` does the check.
ACKs for top commit:
theStack:
re-ACK ee045b61ef
rkrux:
re-ACK ee045b61ef
fjahr:
Code review ACK ee045b61ef
Tree-SHA512: 4ead5be1ef6756251b827f594beba868a145d75bf7f4ef6f15ad21f0ae4b8d71b38c83494e5a6b75f37fadd097178cddd93d614b962a2c72fc134f00ba2f74ae
We don't add or maintain these, and they are of little value, as
well as having the effect of polluting diffs.
They are also wrong, i.e DEFAULT_SCRIPTCHECK_THREADS is not in
validation.h.
0671d66a8e wallet, refactor: Convert uint256 to Txid in wallet (marcofleon)
c8ed51e62b wallet, refactor: Convert uint256 to Txid in wallet interfaces (marcofleon)
b3214cefe6 qt, refactor: Convert uint256 to Txid in the GUI (marcofleon)
Pull request description:
This is part of https://github.com/bitcoin/bitcoin/pull/32189.
Converts all instances of transactions from `uint256` to `Txid` in the wallet, GUI, and related interfaces.
ACKs for top commit:
stickies-v:
re-ACK 0671d66a8e, no changes since 65fcfbb2b38bef20a58daa6c828c51890180611d except rebase.
achow101:
ACK 0671d66a8e
furszy:
Code review ACK 0671d66a8e
Tree-SHA512: 9fd4675db63195c4eed2d14c25015a1821fb597f51404674e4879a44a9cf18f475021a97c5f62f3926b7783ade5a38567386f663acba9f5861f1f59c1309ed60
Instead of having the caller have to figure out the correct sane default
to provide to FillPSBT, have FillPSBT do that by having it take the
sighash type as an optional. This further allows it to distinguish
between an explicit sighash type being provided and expecting the
default value to be used.
It is confusing that the chain client flush happens between
StopHTTPServer and StopMapPort. Also, it is unused code. Seems best to
just add it back properly when it is needed again.
In most cases throughout the wallet, the implicit conversion from `Txid` to
`const uint256&` works. However, `commitBumpTransaction` requires a `uint256&`
out parameter, so `bumped_txid` in `feebumper::CommitTransaction` is also
updated here to use `Txid`.
05117e6e17 rpc: clarify longpoll behavior (Sjors Provoost)
5315278e7c Have createNewBlock() wait for a tip (Sjors Provoost)
64a2795fd4 rpc: handle shutdown during long poll and wait methods (Sjors Provoost)
a3bf43343f rpc: drop unneeded IsRPCRunning() guards (Sjors Provoost)
f9cf8bd0ab Handle negative timeout for waitTipChanged() (Sjors Provoost)
Pull request description:
This PR prevents Mining interface methods from sometimes crashing when called during startup before a tip is connected. It also makes other improvements like making more RPC methods usable from the GUI. Specifically this PR:
- Adds an `Assume` check to disallow passing negative timeout values to `Mining::waitTipChanged`
- Makes `waitfornewblock`, `waitforblock` and `waitforblockheight` RPC methods usable from the GUI when `-server=1` is not set.
- Changes `Mining::waitTipChanged` to return `optional<BlockRef>` instead of `BlockRef` and return `nullopt` instead of crashing if there is a timeout or if the node is shut down before a tip is connected.
- Changes `Mining::waitTipChanged` to not time out before a tip is connected, so it is convenient and safe to call during startup, and only returns `nullopt` on early shutdowns.
- Changes `Mining::createNewBlock` to block and wait for a tip to be connected if it is called on startup instead of crashing. Also documents that it will return null on early shutdowns.
This allows `waitNext()` (added in https://github.com/bitcoin/bitcoin/pull/31283) to safely assume `TipBlock()` isn't `null`, not even during a scenario of early shutdown.
Finally this PR clarifies long poll behaviour, mostly by adding code comments, but also through an early `break`.
ACKs for top commit:
achow101:
ACK 05117e6e17
ryanofsky:
Code review ACK 05117e6e17, just updated a commit message since last review
TheCharlatan:
ACK 05117e6e17
vasild:
ACK 05117e6e17
Tree-SHA512: 277c285a6e73dfff88fd379298190b264254996f98b93c91c062986ab35c2aa5e1fbfec4cd71d7b29dc2d68e33f252b5cfc501345f54939d6bd78599b71fec04
226d81f8b7 mining: drop unused -nFees and sigops from CBlockTemplate (Sjors Provoost)
53ad845fb9 test: check fees and sigops in getblocktemplate (Sjors Provoost)
Pull request description:
For the coinbase `vTxFees` used a dummy value of -nFees.
Similarly the first `vTxSigOpsCost` entry was calculated from
the dummy coinbase transaction.
This was introduced in #2115, but the values were never returned by the RPC or used in a test.
Drop 'm and add code comments to prevent confusion.
This PR also adds test coverage for the `fees` and `sigops` fields in `getblocktemplate`, so it closes#32053.
ACKs for top commit:
ismaelsadeeq:
re-ACK 226d81f8b7
ryanofsky:
Code review ACK 226d81f8b7. New test was added since last review, which seems very cleanly written and fixes some missing coverage.
glozow:
ACK 226d81f8b7
Tree-SHA512: 79c534e6bc4810d29114b04dd6db798877732cb473e773bf3cc28f83d14ee3982392587bd0baa39857bd53a79eae3b730d7a7029b08a9b6c3b5c51f86657ca5d
- return null on shutdown instead of the last tip
- ignore timeout value node initialization
This allows consumers of BlockTemplate to safely
assume that a tip is connected, instead of having
to account for startup and early shutdown scenarios.
4cd95a2921 refactor: modernize remaining outdated trait patterns (Lőrinc)
ab2b67fce2 scripted-diff: modernize outdated trait patterns - values (Lőrinc)
8327889f35 scripted-diff: modernize outdated trait patterns - types (Lőrinc)
Pull request description:
The use of [`std::underlying_type_t<T>`](https://en.cppreference.com/w/cpp/types/underlying_type) or [`std::is_enum_v<T>`](https://en.cppreference.com/w/cpp/types/is_enum) (and similar ones, introduced in C++14) replace the `typename std::underlying_type<T>::type` and `std::is_enum<T>::value` constructs (available in C++11).
The `_t` and `_v` helper alias templates offer a more concise way to extract the type and value directly.
I've modified the instances I found in the codebase one-by-one (noticed them while investigating https://github.com/bitcoin/bitcoin/pull/31868), and afterwards extracted scripted diff commits to do the trivial ones automatically.
The last commit contains the values that were easier done manually.
I've excluded changes from `src/bench/nanobench.h`, `src/leveldb`, `src/minisketch`, `src/span.h` and `src/sync.h` - let me know if you think they should be included instead.
A few of the code changes can also be reproduced by clang-tidy (but not all of them):
```bash
cmake -B build -DCMAKE_C_COMPILER=clang -DCMAKE_CXX_COMPILER=clang++ -DCMAKE_EXPORT_COMPILE_COMMANDS=ON -DBUILD_BENCH=ON -DBUILD_FUZZ_BINARY=ON -DBUILD_FOR_FUZZING=ON && cmake --build build -j$(nproc)
run-clang-tidy -quiet -p build -j $(nproc) -checks='-*,modernize-type-traits' -fix $(git grep -lE '::(value|type)' ./src ':(exclude)src/bench/nanobench.h' ':(exclude)src/leveldb' ':(exclude)src/minisketch' ':(exclude)src/span.h' ':(exclude)src/sync.h')
```
ACKs for top commit:
laanwj:
Concept and code review ACK 4cd95a2921
Tree-SHA512: a4bcf0f267c0f4e02983b4d548ed6f58d464ec379ac5cd1f998b9ec0cf698b53a9f2557a05a342b661f1d94adefc9a0ce2dc8f764d49453aaea95451e2c4c581
The waitTipChanged() now returns nullopt if the node is shutting down.
Previously it would return the last known tip during shutdown, but
this creates an ambiguous circumstance in the scenario where the
node is started and quickly shutdown, before notifications().TipBlock()
is set.
The getblocktemplate, waitfornewblock and waitforblockheight RPC
are updated to handle this. Existing behavior is preserved.
Co-authored-by: Ryan Ofsky <ryan@ofsky.org>
For the coinbase vTxFees used a dummy value of -nFees. This
value was never returned by the RPC or used in a test.
Similarly the fist vTxSigOpsCost entry was calculated from
the dummy coinbase transaction.
Drop both and add code comments to prevent confusion.
The use of e.g. `std::underlying_type_t<T>` replaces the older `typename std::underlying_type<T>::type`.
The `_t` helper alias template (such as `std::underlying_type_t<T>`) introduced in C++14 offers a cleaner and more concise way to extract the type directly.
See https://en.cppreference.com/w/cpp/types/underlying_type for details.
-BEGIN VERIFY SCRIPT-
sed -i -E 's/(typename )?(std::[a-z_]+)(<[^<>]+>)::type\b/\2_t\3/g' $(git grep -l '::type' ./src ':(exclude)src/bench/nanobench.h' ':(exclude)src/leveldb' ':(exclude)src/minisketch' ':(exclude)src/span.h' ':(exclude)src/sync.h')
-END VERIFY SCRIPT-
70398ae05b mapport: make ProcessPCP void (Antoine Poinsot)
9e6cba2988 mapport: remove unnecessary 'g_mapport_enabled' (Antoine Poinsot)
8fb45fcda0 mapport: remove unnecessary 'g_mapport_current' variable (Antoine Poinsot)
1b223cb19b mapport: merge DispatchMapPort into StartMapPort (Antoine Poinsot)
9bd936fa34 mapport: drop unnecessary function (Antoine Poinsot)
2a6536ceda mapport: rename 'use_pcp' to 'enable' (Antoine Poinsot)
c4e82b854c mapport: make 'enabled' and 'current' bool (Antoine Poinsot)
Pull request description:
Followup to #31130, this does a couple cleanups to `src/mapport.*` to clarify the logic now that there is a single protocol option for port mapping.
ACKs for top commit:
laanwj:
Code review ACK 70398ae05b
TheCharlatan:
ACK 70398ae05b
Tree-SHA512: d9a3ab4fcd59a7cf4872415c40cc7ac3a98dfc5aa25e195d4df880bb588bac286c30c3471e9d9499de379a75f45dcd0a82019eba3cb9f342004ae1482d0ba075
processNewBlock was added in 7b4d3249ce, but became unnecessary with the introduction of interfaces::BlockTemplate::submitSolution in 7b4d3249ce.
getTransactionsUpdated() is only needed by the implementation of waitFeesChanged() (not yet part of the interface).
f86678156a Check leaves size maximum in MerkleComputation (Sjors Provoost)
4d57288246 refactor: use CTransactionRef in submitSolution (Sjors Provoost)
2e81791d90 Drop TransactionMerklePath default position arg (Sjors Provoost)
39d3b538e6 Rename merkle branch to path (Sjors Provoost)
Pull request description:
This PR implements the refactors suggested in https://github.com/bitcoin/bitcoin/pull/30955#pullrequestreview-2354931253.
ACKs for top commit:
tdb3:
code review re-ACK f86678156a
itornaza:
re ACK f86678156a
ryanofsky:
Code review ACK f86678156a only changes since last review are a whitespace change and adding an Assume statement to check for size_t -> uint32_t overflows
Tree-SHA512: 661b5d5d0e24b2269bf33ab1484e37c36e67b32a7796d77ca3b1856d3043378b081ad43c32a8638b46fa8c0de51c823fd9747dd9fc81f958f20d327bf330a47c
52fd1511a7 test: drop scriptPubKeyIn arg from CreateNewBlock (Sjors Provoost)
ff41b9e296 Drop script_pub_key arg from createNewBlock (Sjors Provoost)
7ab733ede4 rpc: rename coinbase_script to coinbase_output_script (Sjors Provoost)
Pull request description:
Providing a script for the coinbase transaction is only done in test code and for (unoptimized) CPU solo mining.
Production miners use the `getblocktemplate` RPC which omits the coinbase transaction entirely from its block template, leaving it to external (pool) software to construct it.
This commit removes the `script_pub_key argument` from `createNewBlock()` in the Mining interface.
A coinbase script can still be passed via `BlockCreateOptions` instead. Tests are modified to do so.
ACKs for top commit:
ryanofsky:
Code review ACK 52fd1511a7. No change since last review other than rebase
TheCharlatan:
Re-ACK 52fd1511a7
vasild:
ACK 52fd1511a7
Tree-SHA512: c4b3a53774d9a5dc90950e77f47a64dbb68f971baffbb9a0d8f59332ef8e52d0c039130c925bde73135b3d0e79e65d91d1df30dc4cff13f32d8a72e5c56669d8
Providing a script for the coinbase transaction is only done in test code
and for CPU solo mining.
Production miners use the getblocktemplate RPC which omits the coinbase
transaction entirely from its block template, leaving it to external (pool)
software to construct it.
A coinbase script can still be passed via BlockCreateOptions instead.
A temporary overload is added so that the test can be modified in the
next commit.
525e9dcba0 Add submitSolution to BlockTemplate interface (Sjors Provoost)
47b4875ef0 Add getCoinbaseMerklePath() to Mining interface (Sjors Provoost)
63d6ad7c89 Move BlockMerkleBranch back to merkle.{h,cpp} (Sjors Provoost)
Pull request description:
The new `BlockTemplate` interface introduced in #30440 allows for a more efficient way for a miner to submit the block solution. Instead of having the send the full block, it only needs to provide the nonce, timestamp, version fields and coinbase transaction.
This PR introduces `submitSolution()` for that. It's currently unused.
#29432 and https://github.com/Sjors/bitcoin/pull/48 use it to process the Stratum v2 message [SubmitSolution](https://github.com/stratum-mining/sv2-spec/blob/main/07-Template-Distribution-Protocol.md#77-submitsolution-client---server). The method should be sufficiently generic to work with alternative mining protocols (none exist that I'm aware off).
This PR also introduces `getCoinbaseMerklePath()`, which is needed in Stratum v2 to construct the `merkle_path` field of the `NewTemplate` message (see [spec](https://github.com/stratum-mining/sv2-spec/blob/main/07-Template-Distribution-Protocol.md#72-newtemplate-server---client)). The coinbase merkle path is also used in Stratum "v1", see e.g. https://bitcoin.stackexchange.com/questions/109820/questions-on-merkle-root-hashing-for-stratum-pools
This last function uses `BlockMerkleBranch` which was moved to the test code in #13191. The reason back then for moving it was that it was no longer used. This PR moves it back.
This PR does not change behaviour since both methods are unused.
ACKs for top commit:
achow101:
ACK 525e9dcba0
itornaza:
Code review ACK 525e9dcba0
tdb3:
Code review and light test ACK 525e9dcba0
ryanofsky:
Code review ACK 525e9dcba0. Left minor suggestions but none are important, and looks like this could be merged as-is
Tree-SHA512: 2a6a8f5d409ff4926643193cb67702240c7c687615414371e53383d2c13c485807f65e21e8ed98515b5456eca3d9fca13cec04675814a4081467d88b849c5653
It should be obvious that a wait is not needed if the tip does not
match.
Also, remove a comment that the blockTip notification was only meant for
the "UI". (It is used by other stuff for a long time)
5c7cacf649 ci: Remove natpmp build option and libnatpmp dependency (laanwj)
7e7ec984da doc: Remove mention of natpmp build options (laanwj)
061c3e32a2 depends: Drop natpmp and associated option from depends (laanwj)
20a18bf6aa build: Drop libnatpmp from build system (laanwj)
7b04709862 qt: Changes for built-in PCP+NAT-PMP (laanwj)
52f8ef66c6 net: Replace libnatpmp with built-in NATPMP+PCP implementation in mapport (laanwj)
97c97177cd net: Add PCP and NATPMP implementation (laanwj)
d72df63d16 net: Use GetLocalAddresses in Discover (laanwj)
e02030432b net: Add netif utility (laanwj)
754e425438 crypto: Add missing WriteBE16 function (laanwj)
Pull request description:
Continues #30005. Closes #17012..
This PR adds PCP (Port Control Protocol) from [RFC6887](https://datatracker.ietf.org/doc/html/rfc6887). This adds, in addition to the existing IPv4 port mapping (which now uses PCP, with fallback to NAT-PMP), support for IPv6 pinholing-that is, opening a port on the firewall to make it reachable.
PCP, like NAT-PMP is a simple UDP-based protocol, and the implementation is self-contained, so this gets rid of lthe libnatpnp dependency without adding a new one. It should otherwise be a drop-in replacement. NAT-PMP fallback is implemented so this will not make router support worse.
For now it is disabled by default, though in the future (not in this PR) we could consider enable it by default to increase the number of connectable nodes without adding significant attack surface.
To test:
```bash
bitcoind -regtest -natpmp=1 -debug=net
```
(most of the changes in this PR are, ironically, removing the libnatpmp dependency and associated build system and build docs)
## TODO
- [x] Default gateway discovery on Linux / FreeBSD
- [x] Default gateway discovery on Windows
- [x] Default gateway discovery on MacOS
- [x] Either solve FreeBSD compile issue (probably upstream issue) or remove FreeBSD support
## Things to consider for follow-up PRs
- https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1658764974 avoid unreachable nets (not given to -onlynet=)
- https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1658949236 could announce an addr:port where we do not listen (no -bind)
- https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1684368824 could announce the wrong port because it uses GetListenPort()
- https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1679709347 if we requested one port but another was assigned, then which one to use in the renewal?
- https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1772017020 Use `GetAdapterAddresses` to discover local addresses for Windows
ACKs for top commit:
Sjors:
ACK 5c7cacf649
achow101:
ACK 5c7cacf649
vasild:
ACK 5c7cacf649
Tree-SHA512: e35b69e56d5f5449a3d48a799f9b7b65107c65eeb3e245c2c1e9d42221e469ca5ead90afae423160601cd664dd553a51c859e04f4492f335b064aae3bf23e3bc
