Age | Commit message (Collapse) | Author |
|
-BEGIN VERIFY SCRIPT-
git grep -l CAddrMan src/ test/ | xargs sed -i 's/CAddrMan/AddrMan/g'
-END VERIFY SCRIPT-
|
|
CAddrInfo objects are an implementation detail of how AddrMan manages and adds
metadata to different records. Encapsulate this logic by updating Select &
SelectTriedCollision to return the additional info that the callers need.
|
|
Improve readability of code, simplify future scripted diff cleanup PRs, and be
more consistent with naming for GetBoolArg.
This will also be useful for replacing runtime settings type checking
with compile time checking.
-BEGIN VERIFY SCRIPT-
git grep -l GetArg | xargs sed -i 's/GetArg(\([^)]*\( [0-9]\+\|-1\|port\|BaseParams().RPCPort()\|Params().GetDefaultPort()\|_TIMEOUT\|Height\|_WORKQUEUE\|_THREADS\|_CONNECTIONS\|LIMIT\|SigOp\|Bytes\|_VERSION\|_AGE\|_CHECKS\|Checks() ? 1 : 0\|_BANTIME\|Cache\|BLOCKS\|LEVEL\|Weight\|Version\|BUFFER\|TARGET\|WEIGHT\|TXN\|TRANSACTIONS\|ADJUSTMENT\|i64\|Size\|nDefault\|_EXPIRY\|HEIGHT\|SIZE\|SNDHWM\|_TIME_MS\)\))/GetIntArg(\1)/g'
-END VERIFY SCRIPT-
Co-authored-by: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com>
|
|
The benchmarks can now run much longer due to the minimum of 10ms or
directly with -min_time. With -min_time=20000 I could trigger two ubsan
errors in the benchmarks, which are fixed in this commit by using
unsigned type and adding "& 0xFF".
|
|
- use ALLOW_BOOL for -list arg instead of ALLOW_ANY
- touch up `-asymptote=<n1,n2,n3...>` help
- pack Args struct a bit more efficiently
- handle args in alphabetical order
|
|
Drops unneeded and adds missing includes
|
|
This adds some usage description with tips to `bench_bitcoin -h`.
|
|
When it is not easily possible to stabilize benchmark machine and code
the argument -min_time can be used to specify a minimum duration
that a benchmark should take. E.g. choose -min_time=1000 if you
are willing to wait about 1 second for each benchmark result.
The default is now set to 10ms instead of 0, which should make runs on
fast machines more stable with negligible slowdown.
|
|
Moves copying of the setup into the benchmark loop so it is possible
to run the loop for an arbitrary number of times.
The overhead due to copying the candidates inside the loop is about 3%.
|
|
Moves some of the setup into the benchmark loop so it is possible to run
the loop for an arbitrary number of times. Due to recent optimizations
in #22974 the benchmark now runs much faster, so the inner loop now calls
Good() 32 times as often to get better numbers.
Renamed the benchmark to AddrManAddThenGood because that's now what is
actually tested. To get the the number of just Good(), one needs to
subtract the benchmark result of AddrManAdd.
|
|
Introduced in #19055, MuHashDiv benchmark used to multiply with a loop
based on epochIterations. That does not do what it is supposed to do,
because epochIterations() is determined automatically from nanobench.
Also, multiplication is not needed for the algorithm (as pointed out by
a comment in #19055), so it's better to remove this loop.
|
|
Most importantly, this update fixes a bug in nanobench that always
disabled performance counters on linux.
It also adds another sanitizer suppression that is caught in clang++ 12.
|
|
fa309ee61c09726a8780acaea94502712f817921 bench: Fix 32-bit compilation failure in addrman bench (MarcoFalke)
fae0295a799499268caca9c385ac4d7061543980 ci: Switch multiprocess to i686 build (MarcoFalke)
Pull request description:
Building for i686 with clang helps to catch bugs early for:
* The OSS-Fuzz i686 clang libFuzzer build
* The arm 32-bit native clang build
Fixes #22889
ACKs for top commit:
hebasto:
ACK fa309ee61c09726a8780acaea94502712f817921
Tree-SHA512: 581820d319aae2fcd4dd44979ee3d4164a575f0438476890aa2a7447f1392a5da26766cd6ab954530499b54f66eec2417bdeefdd7efb19bc27dd679cd2b9d0ce
|
|
|
|
|
|
724c4975622bc22cedc3f3814dfc8e66cf8371f7 [fuzz] Add ConsumeAsmap() function (John Newbery)
5840476714ffebb2599999c85a23b52ebcff6090 [addrman] Make m_asmap private (John Newbery)
f9002cb5dbd573cd9ca200de21319fa296e26055 [net] Rename the copyStats arg from m_asmap to asmap (John Newbery)
f572f2b2048994b3b50f4cfd5de19e40b1acfb22 [addrman] Set m_asmap in CAddrMan initializer list (John Newbery)
593247872decd6d483a76e96d79433247226ad14 [net] Remove CConnMan::SetAsmap() (John Newbery)
50fd77045e2f858a53486b5e02e1798c92ab946c [init] Read/decode asmap before constructing addrman (John Newbery)
Pull request description:
Commit 181a1207 introduced an initialization order bug: CAddrMan's m_asmap must be set before deserializing peers.dat.
The first commit restores the correct initialization order. The remaining commits make `CAddrMan::m_asmap` usage safer:
- don't reach into `CAddrMan`'s internal data from `CConnMan`
- set `m_asmap` in the initializer list and make it const
- make `m_asmap` private, and access it (as a reference to const) from a getter.
This ensures that peers.dat deserialization must happen after setting m_asmap, since m_asmap is set during CAddrMan construction.
ACKs for top commit:
mzumsande:
Tested ACK 724c4975622bc22cedc3f3814dfc8e66cf8371f7
amitiuttarwar:
code review but utACK 724c497562
naumenkogs:
utACK 724c4975622bc22cedc3f3814dfc8e66cf8371f7
vasild:
ACK 724c4975622bc22cedc3f3814dfc8e66cf8371f7
MarcoFalke:
review ACK 724c4975622bc22cedc3f3814dfc8e66cf8371f7 👫
Tree-SHA512: 684a4cf9e3d4496c9997fb2bc4ec874809987055c157ec3fad1d2143b8223df52b5a0af787d028930b27388c8efeba0aeb2446cb35c337a5552ae76112ade726
|
|
Followup to commit "MOVEONLY: CWallet transaction code out of
wallet.cpp/.h" that detaches and renames some CWalletTx methods, making
into them into standalone functions or CWallet methods instead.
There are no changes in behavior and no code changes that aren't purely
mechanical. It just gives spend and receive functions more consistent
names and removes the circular dependencies added by the earlier
MOVEONLY commit.
There are also no comment or documentation changes. Removed comments
from transaction.h are just migrated to spend.h, receive.h, and
wallet.h.
|
|
This allows us to make it const.
|
|
Just use unique_ptr<CAddrMan>s and reset the pointer if a frest addrman is required.
Also make CAddrMan::Clear() private to ensure that no call sites are missed.
|
|
Currently addrman consistency checks are a compile time option, and are not
enabled in our CI. It's unlikely anyone is running these consistency checks.
Make them a runtime option instead, where users can enable addrman
consistency checks every n operations (similar to mempool tests). Update
the addrman unit tests to do internal consistency checks every 100
operations (checking on every operations causes the test runtime to
increase by several seconds).
Also assert on a failed addrman consistency check to terminate program
execution.
|
|
Removes the need for tests to update nKey and insecure_rand after constructing
a CAddrMan.
|
|
instead of signed int
fa621ededdfe31a200b77a8787de7e3d2e667aec refactor: Pass script verify flags as uint32_t (MarcoFalke)
Pull request description:
The flags are cast to unsigned in the interpreter anyway, so avoid the confusion (and fuzz crashes) by just passing them as unsigned from the beginning.
Also, the flags are often inverted bit-wise with the `~` operator, which also works on signed integers, but might cause confusion as the sign bit is flipped.
Fixes #22233
ACKs for top commit:
theStack:
Concept and code review ACK fa621ededdfe31a200b77a8787de7e3d2e667aec
kristapsk:
ACK fa621ededdfe31a200b77a8787de7e3d2e667aec
jonatack:
ACK fa621ededdfe31a200b77a8787de7e3d2e667aec
Tree-SHA512: ea0720f32f823fa7f075309978672aa39773c6019d12b6c1c9d611fc1983a76115b7fe2a28d50814673bb6415c311ccc05b99d6e871575fb6900faf75ed17769
|
|
|
|
Co-authored-by: Vasil Dimov <vd@FreeBSD.org>
|
|
- remove unneeded strprintf
- consistent punctuation (no EOL periods)
- sort helps by order they are printed (alphabetical order)
|
|
|
|
They are cast to unsigned anyway when calling VerifyScript,
bitcoinconsensus_verify_script*, or CountWitnessSigOps.
|
|
-BEGIN VERIFY SCRIPT-
find_regex='((assert|CHECK_NONFATAL)\(std::addressof|TODO: REVIEW-ONLY)' \
&& git grep -l -E "$find_regex" -- . \
| xargs sed -i -E "/${find_regex}/d"
-END VERIFY SCRIPT-
|
|
CreateTransactionInternal
96c2c9520e80ee4fed92f0e1ab859d59fcbdb110 scripted-diff: Rename SelectCoinsMinConf to AttemptSelection (Andrew Chow)
b583f73354c617ede9145f9738f13cedf1c13e08 Move vin filling to before final fee setting (Andrew Chow)
d39cac0547c960df0a890e89f43b458147b4b07a Set m_subtract_fee_outputs during recipients vector loop (Andrew Chow)
364e0698a543a19e81ae407cc523970e6ed924e8 Move variable initializations to where they are used (Andrew Chow)
32ab430651594ed3d10a6ed75f19de5197f0e9b0 Move recipients vector checks to beginning of CreateTransaction (Andrew Chow)
cd1d6d3324a841087f6d5da723394e8d7df07ec7 Rename nSubtractFeeFromAmount in CreateTransaction (Andrew Chow)
dac21c793f8fbb4d5debc55ac97c406c7c93ff48 Rename nValue and nValueToSelect (Andrew Chow)
d2aee3bbc765a1f02e4ceadb2fa5928ac524f1a7 Remove extraneous scope in CreateTransactionInternal (Andrew Chow)
b2995963b5d0b9bca503b0cc69c747f4cedec1e4 Move cs_wallet lock in CreateTransactionInternal to top of function (Andrew Chow)
Pull request description:
#17331 did some refactors and cleanup of `CreateTransactionInternal` to make it easier to understand, however it is still a bit convoluted even though it doesn't have to be. This PR does additional cleanup and refactoring to `CreateTransactionInternal` so that it is easier to understand. Some unnecessary code was removed, some variables moved around to where they matter, and several indents removed.
ACKs for top commit:
glozow:
reACK 96c2c95
ryanofsky:
Code review ACK 96c2c9520e80ee4fed92f0e1ab859d59fcbdb110 also acked previously (was reverted).
meshcollider:
re-utACK 96c2c9520e80ee4fed92f0e1ab859d59fcbdb110
Tree-SHA512: 3dba67ed436968a07bfd82d435d566ad74e116c6e50ac9baed7144a46ad5c0f630b1ba59d91e8e8972ac2af559d7c0576f0560f09684d2ab20fad6689902866f
|
|
44d05d0a69c14ed295b0a7f6c8ec4379d44155e4 test: remove sanitizer suppression for nanobench (Martin Ankerl)
e3c866e3ca85f841671a828712e6207e24d0d996 test: update nanobench from release 4.0.0 to 4.3.4 (Martin Ankerl)
Pull request description:
This updates the third-party library nanobench with the latest release. It contains mostly minor bugfixes, a new pyperf output format, ability to suppress warnings with environment variable `NANOBENCH_SUPPRESS_WARNINGS`. Full changelog:
v4.0.2
* Changed `doNotOptimizeAway` to what google benchmark is doing. The old code did not work on some machines.
* fix: display correct "total" value
* minor Documentation updates
v4.1.0
* Updated link to new pyperf home
* Adds ability to configure console output time unit
* Add support for environment variable `NANOBENCH_SUPPRESS_WARNINGS`
* Nanobench is now usable with CMake's FetchContent (see documentation: https://nanobench.ankerl.com/tutorial.html#cmake-integration)
v4.2.0
* Ability to store and later compare results added, through `pyperf`.
* See https://nanobench.ankerl.com/tutorial.html#pyperf-python-pyperf-module-output
* Added lots of build targets to travis, similar to bitcoin's build.
* Some minor API & documentation improvements
v4.3.0
* `ankerl::nanobench::Rng` can now return the state with `std::vector<uint64_t> Rng::state()`, and this can also be used to initialize the Rng.
v4.3.1
* Minor cmake improvements when integrationg as a third-party library: add alias `nanobench::nanobench`, default to C++17
v4.3.2
* Fixed a MSVC 2015 build problem
* updates license to 2021.
* build should now work with very old linux headers
* Also disable UBSAN (bitcoin needed to add a suppression)
v4.3.3
* Do not use locale-dependent `std::to_string`
v4.3.4
* Add missing sanitizer suppression to `rotl`
ACKs for top commit:
MarcoFalke:
review ACK 44d05d0a69c14ed295b0a7f6c8ec4379d44155e4
Tree-SHA512: 3291c85057720cfc84a44bfaa305a7d0df4dc35779169d20de73d32e40d4cdbf3f005bf343f79710eca517441de2459e8118c195c5f5136f99d1f50ebd5dfd08
|
|
auxiliary modules
7a799c9c2b8652e780d1fd5e1bf7d05b026c1c1a index: refactor-only: Reuse CChain ref (Carl Dong)
db33cde80fff749c6adff9e91fca5f27f4bb6278 index: Add chainstate member to BaseIndex (Carl Dong)
f4a47a1febfa35ab077f2a841fe31a8cd9618250 bench: Use existing chainman in AssembleBlock (Carl Dong)
91226eb91769aad5a63bc671595e1353a2b2247a bench: Use existing NodeContext in DuplicateInputs (Carl Dong)
e6b4aa6eb53dc555ecab2922af35e7a2572faf4f miner: Pass in chainman to RegenerateCommitments (Carl Dong)
9ecade14252ad1972f668d2d2e4ef44fdfcb944a rest: Add GetChainman function and use it (Carl Dong)
fc1c282845f6b8436d1ea4c68eb3511034c29bea rpc/blockchain: Use existing blockman in gettxoutsetinfo (Carl Dong)
Pull request description:
Overall PR: #20158 (tree-wide: De-globalize ChainstateManager)
The first 2 commits are fixups addressing review for the last bundle: #21391
NEW note:
1. I have opened #21766 which keeps track of potential improvements where the flaws already existed before the de-globalization work, please post on that issue about these improvements, thanks!
Note to reviewers:
1. This bundle may _apparently_ introduce usage of `g_chainman` or `::Chain(state|)Active()` globals, but these are resolved later on in the overall PR. [Commits of overall PR](https://github.com/bitcoin/bitcoin/pull/20158/commits)
2. There may be seemingly obvious local references to `ChainstateManager` or other validation objects which are not being used in callers of the current function in question, this is done intentionally to **_keep each commit centered around one function/method_** to ease review and to make the overall change systematic. We don't assume anything about our callers. Rest assured that once we are considering that particular caller in later commits, we will use the obvious local references. [Commits of overall PR](https://github.com/bitcoin/bitcoin/pull/20158/commits)
3. When changing a function/method that has many callers (e.g. `LookupBlockIndex` with 55 callers), it is sometimes easier (and less error-prone) to use a scripted-diff. When doing so, there will be 3 commits in sequence so that every commit compiles like so:
1. Add `new_function`, make `old_function` a wrapper of `new_function`, divert all calls to `old_function` to `new_function` **in the local module only**
2. Scripted-diff to divert all calls to `old_function` to `new_function` **in the rest of the codebase**
3. Remove `old_function`
ACKs for top commit:
jarolrod:
ACK 7a799c9
ariard:
Code Review ACK 7a799c9
fjahr:
re-ACK 7a799c9c2b8652e780d1fd5e1bf7d05b026c1c1a
MarcoFalke:
review ACK 7a799c9c2b8652e780d1fd5e1bf7d05b026c1c1a ðŸŒ
ryanofsky:
Code review ACK 7a799c9c2b8652e780d1fd5e1bf7d05b026c1c1a. Basically no change since last review except fixed rebase conflicts and a new comment about REST Ensure()
jamesob:
conditional ACK 7a799c9c2b8652e780d1fd5e1bf7d05b026c1c1a ([`jamesob/ackr/21767.1.dongcarl.bundle_6_n_prune_g_chai`](https://github.com/jamesob/bitcoin/tree/ackr/21767.1.dongcarl.bundle_6_n_prune_g_chai))
Tree-SHA512: 531c00ddcb318817457db2812d9a9d930bc664e58e6f7f1c746350732b031dd624270bfa6b9f49d8056aeb6321d973f0e38e4ff914acd6768edd8602c017d10e
|
|
This updates the third-party library nanobench with the latest release. It contains mostly minor bugfixes, a new pyperf output format, ability to suppress warnings with environment variable `NANOBENCH_SUPPRESS_WARNINGS`. Full changelog:
v4.0.2
* Changed `doNotOptimizeAway` to what google benchmark is doing. The old code did not work on some machines.
* fix: display correct "total" value
* minor Documentation updates
v4.1.0
* Updated link to new pyperf home
* Adds ability to configure console output time unit
* Add support for environment variable `NANOBENCH_SUPPRESS_WARNINGS`
* Nanobench is now usable with CMake's FetchContent (see documentation: https://nanobench.ankerl.com/tutorial.html#cmake-integration)
v4.2.0
* Ability to store and later compare results added, through `pyperf`.
* See https://nanobench.ankerl.com/tutorial.html#pyperf-python-pyperf-module-output
* Added lots of build targets to travis, similar to bitcoin's build.
* Some minor API & documentation improvements
v4.3.0
* `ankerl::nanobench::Rng` can now return the state with `std::vector<uint64_t> Rng::state()`, and this can also be used to initialize the Rng.
v4.3.1
* Minor cmake improvements when integrationg as a third-party library: add alias `nanobench::nanobench`, default to C++17
v4.3.2
* Fixed a MSVC 2015 build problem
* updates license to 2021.
* build should now work with very old linux headers
* Also disable UBSAN (bitcoin needed to add a suppression)
v4.3.3
* Do not use locale-dependent `std::to_string`
v4.3.4
* Add missing sanitizer suppression to `rotl`
|
|
SelectCoinsMinConf is a bit of a misnomer now since it really just does
all of the coin selection given some parameters. So rename this to
something less annoying to say and makes a bit more sense.
-BEGIN VERIFY SCRIPT-
sed -i 's/SelectCoinsMinConf/AttemptSelection/g' $(git grep -l SelectCoinsMinConf ./src)
-END VERIFY SCRIPT-
|
|
|
|
|
|
51a3ac242c92e69b59df26f8f9e287b31e5c3b0f Have OutputGroup determine the value to use (Andrew Chow)
6d6d2784759878ef0c4ac128d12aac68add1edca Change SelectCoins_test to actually test SelectCoins (Andrew Chow)
9d3bd74ab4430532d6e53eef8cf77ad999044b14 Remove CreateTransaction while loop and some related variables (Andrew Chow)
6f0d5189af4c881fe8b97a0c28ce1ffa33480715 Remove use_bnb and bnb_used (Andrew Chow)
de26eb0e1fa2b6f03c58ba104d00f7a8ffead39c Do both BnB and Knapsack coin selection in SelectCoinsMinConf (Andrew Chow)
01dc8ebda50a382d45d3d169b2c3f3965869dcae Have KnapsackSolver actually use effective values (Andrew Chow)
bf26e018de33216d6f0ed0d6ff822b93536f7cc1 Roll static tx fees into nValueToSelect instead of having it be separate (Andrew Chow)
cc3f14b27c06b7a0da1472f5c7100c3f0b76fd98 Move output reductions for fee to after coin selection (Andrew Chow)
d97d25d95006725e705635530b27643363d6b2a4 Make cost_of_change part of CoinSelectionParams (Andrew Chow)
af5867c89688b06173b295b7c32a42845ea455da Move some calculations to common code in SelectCoinsMinConf (Andrew Chow)
1bf4a62cb61bd4b91d9cd4e379fea2b914786342 scripted-diff: rename some variables (Andrew Chow)
Pull request description:
Changes `KnapsackSolver` to use effective values instead of just the nominal txout value. Since fees are taken into account during the selection itself, we finally get rid of the `CreateTransaction` loop as well as a few other things that only were only necessary because of that loop.
This should not change coin selection behavior at all (except maybe remove weird edge cases that were caused by the loop). In order to keep behavior the same, `KnapsackSolver` will select outputs with a negative effective value (as it did before).
ACKs for top commit:
ryanofsky:
Code review ACK 51a3ac242c92e69b59df26f8f9e287b31e5c3b0f. Looks good to go!
instagibbs:
review ACK https://github.com/bitcoin/bitcoin/pull/17331/commits/51a3ac242c92e69b59df26f8f9e287b31e5c3b0f
meshcollider:
re-light-utACK 51a3ac242c92e69b59df26f8f9e287b31e5c3b0f
Tree-SHA512: 372c27e00edcd5dbf85177421ba88f20bfdaf1791b6e3dc022c44876ecc379403e2375ed69e71c512c49e6af87641001ff385c4b25ab93684b3a08a53bf3824e
|
|
getnodeaddresses by network
ce6bca88e8c685c69686e0b8dc095ffc3e2ac34d doc: release note for getnodeaddresses by network (Jon Atack)
3f89c0e9902338ad8a507a938dceeeb3191eece6 test: improve getnodeaddresses coverage, test by network (Jon Atack)
6c98c099918bd20e2d3aa123643d6e3594e080e4 rpc: enable filtering getnodeaddresses by network (Jon Atack)
80ba294854e5025bcada58f1403858e6ea1d4380 p2p: allow CConnman::GetAddresses() by network, add doxygen (Jon Atack)
a49f3ddbbabfb971a537f0a6c7affb24e20ff192 p2p: allow CAddrMan::GetAddr() by network, add doxygen (Jon Atack)
c38981e748f438d972ba12ba998c8a8a597e01c1 p2p: pull time call out of loop in CAddrMan::GetAddr_() (João Barbosa)
d35ddca91ebbcf8d8b790c3b9f8cf218fafb7a53 p2p: enable CAddrMan::GetAddr_() by network, add doxygen (Jon Atack)
Pull request description:
This patch allows passing a network argument to CAddrMan::GetAddr(), CConnman::GetAddresses(), and rpc getnodeaddresses to return only addresses of that network.
It also contains a performance optimisation by promag.
ACKs for top commit:
laanwj:
Code review and lightly tested ACK ce6bca88e8c685c69686e0b8dc095ffc3e2ac34d
vasild:
ACK ce6bca88e8c685c69686e0b8dc095ffc3e2ac34d
Tree-SHA512: 40e700d97091248429c73cbc0639a1f03ab7288e636a7b9026ad253e9708253c6b2ec98e7d9fb2d56136c0f762313dd648915ac98d723ee330d713813a43f99d
|
|
These booleans are no longer needed
|
|
The fees for transaction overhead and recipient outputs are now included
in nTargetValue instead of being a separate parameter. For the coin
selection algorithms, it doesn't matter that these are separate as in
either case, the algorithm needs to select enough to cover these fees.
Note that setting nValueToSelect is changed as it now includes
not_input_fees. Without the change to how nValueToSelect is increased
for KnapsackSolver, this would result in overpaying fees. The change to
increase by the difference between nFeeRet and not_input_fees allows
this to have the same behavior as previously.
Additionally, because we assume that KnapsackSolver will always find a
solution that requires change (we assume that BnB always finds a
non-change solution), we also include the fee for the change output in
KnapsackSolver's target. As part of this, we also use the changeless
nFeeRet when iterating for KnapsackSolver. This is because we include
the change fee when doing KnapsackSolver, so nFeeRet on further
iterations won't include the change fee.
|
|
|
|
|
|
|
|
725d7ae0494d4a45f5a840bbbd19c008a7363965 Use PrecomputedTransactionData in signet check (Pieter Wuille)
497718b467330b2c6bb0d44786020c55f1aa75f9 Treat amount<0 also as missing data for P2WPKH/P2WSH (Pieter Wuille)
3820090bd619ac85ab35eff376c03136fe4a9f04 Make all SignatureChecker explicit about missing data (Pieter Wuille)
b77b0cc507bdc716e5236b1d9880e648147e0af9 Add MissingDataBehavior and make TransactionSignatureChecker handle it (Pieter Wuille)
Pull request description:
Currently we have 2 levels of potentially-missing data in the transaction signature hashes:
* P2WPKH/P2WSH hashes need the spent amount
* P2TR hashes need all spent outputs (amount + scriptPubKey)
Missing amounts are treated as -1 (thus leading to unexpected signature failures), while missing outputs in P2TR validation cause assertion failure. This is hard to extend for signing support, and also quite ugly in general.
In this PR, an explicit configuration option to {Mutable,}TransactionSignatureChecker is added (MissingDataBehavior enum class) to either select ASSERT_FAIL or FAIL. Validation code passes ASSERT_FAIL (as at validation time all data should always be passed, and anything else is a serious bug in the code), while signing code uses FAIL.
The existence of the ASSERT_FAIL option is really just an abundance of caution. Always using FAIL should be just fine, but if there were for some reason a code path in consensus code was introduced that misses certain data, I think we prefer as assertion failure over silently introducing a consensus change.
Potentially useful follow-ups (not for this PR, in my preference):
* Having an explicit script validation error code for missing data.
* Having a MissingDataBehavior::SUCCEED option as well, for use in script/sign.cpp DataFromTransaction (if a signature is present in a witness, and we don't have enough data to fully validate it, we should probably treat it as valid and not touch it).
ACKs for top commit:
sanket1729:
reACK 725d7ae0494d4a45f5a840bbbd19c008a7363965
Sjors:
ACK 725d7ae0494d4a45f5a840bbbd19c008a7363965
achow101:
re-ACK 725d7ae0494d4a45f5a840bbbd19c008a7363965
benthecarman:
ACK 725d7ae0494d4a45f5a840bbbd19c008a7363965
fjahr:
Code review ACK 725d7ae0494d4a45f5a840bbbd19c008a7363965
Tree-SHA512: d67dc51bae9ca7ef6eb9acccefd682529f397830f77d74cd305500a081ef55aede0e9fa380648c3a8dd4857aa7eeb1ab54fe808979d79db0784ac94ceb31b657
|
|
Remove the implicit MissingDataBehavior::ASSERT_FAIL in the
*TransationSignatureChecker constructors, and instead specify
it explicit in all call sites:
* Test code uses ASSERT_FAIL
* Validation uses ASSERT_FAIL (through CachingTransactionSignatureChecker)
(including signet)
* libconsensus uses FAIL, matching the existing behavior of the
non-amount API (and the extended required data for taproot validation
is not available yet)
* Signing code uses FAIL
|
|
03346022d611871f2cc185440b19d928b9264d9d naming nits (Fabian Jahr)
2e7c80fb5be82ad4a3f737cab65b31f70a772a23 Add signet support to gen_key_io_test_vectors.py (Pieter Wuille)
fe5e495c31de47b0ec732b943db11fe345d874af Use Bech32m encoding for v1+ segwit addresses (Pieter Wuille)
25b1c6e13ddf1626210d5e3d37298d1f3a78a94f Add Bech32m test vectors (Pieter Wuille)
da2bb6976dadeec682d163c258c9afecc87d6428 Implement Bech32m encoding/decoding (Pieter Wuille)
Pull request description:
This implements [BIP 350](https://github.com/bitcoin/bips/blob/master/bip-0350.mediawiki):
* For segwit v1+ addresses, a new checksum algorithm called Bech32m is used.
* Segwit v0 address keep using Bech32 as specified in [BIP 173](https://github.com/bitcoin/bips/blob/master/bip-0173.mediawiki).
ACKs for top commit:
Sjors:
utACK 0334602
jnewbery:
utACK 03346022d6
achow101:
ACK 0334602
fjahr:
re-ACK 0334602
benthecarman:
ACK 03346022d611871f2cc185440b19d928b9264d9d
Tree-SHA512: 4424cfd44869d813d6152fb3ed867b204036736bc2344a039b93700b6f36a43e9110478f138eb81c97c77ab27ecb776dada5ba632cb5a3a9d244924d2540a557
|
|
ebc4ab721b0371c0ef217c0f5bd7d42613e951e6 refactor: post Optional<> removal cleanups (fanquake)
57e980d13ca488031bde6ef197cf34d493d36796 scripted-diff: remove Optional & nullopt (fanquake)
Pull request description:
Same rationale & motivation as #21404, which turned out to be quite low in the number of potential conflicts. Lets see what the bot has to say here.
ACKs for top commit:
practicalswift:
cr ACK ebc4ab721b0371c0ef217c0f5bd7d42613e951e6: patch looks correct
jnewbery:
utACK ebc4ab721b0371c0ef217c0f5bd7d42613e951e6
laanwj:
Code review ACK ebc4ab721b0371c0ef217c0f5bd7d42613e951e6
Tree-SHA512: 550fbeef09b9d35ddefaa805d1755c18c8fd499c4b0f77ebfece8c20296a7abd1cf6c699e2261f92fe3552deeb7555ec2a2287ffe3ab9e98bb9f8612a4d43be3
|
|
|
|
selection
f9cd2bfbccb7a2b8ff07cec5f6d2adbeca5f07c3 Rename CoinSelectionParams::effective_fee to m_effective_feerate (Andrew Chow)
bdd0c2934b7f389ffcfae3b602ee3ecee8581acd wallet: Move discard feerate fetching to CreateTransaction (Andrew Chow)
448d04b931f86941903e855f831249ff5ec77485 wallet: Move long term feerate setting to CreateTransaction (Andrew Chow)
e2f429e6bbf7098f278c0247b954ecd3ba53cf37 wallet: Replace nFeeRateNeeded with effective_fee (Andrew Chow)
1a6a0b0dfb90f9ebd4b86d7934c6aa5594974f5f wallet: Use existing feerate instead of getting a new one (Andrew Chow)
Pull request description:
During coin selection, there are various places where we need to have a feerate. We need the feerate for the transaction itself, the discard fee rate, and long term feerate. Fetching these each time we need them can lead to a race condition where two feerates that should be the same are actually different. One particular instance where this can happen is during the loop in `CreateTransactionInternal`. After inputs are chosen, the expected transaction fee is calculated using a newly fetched feerate. If `pick_new_inputs == false`, the loop will go again with the assumption that the fee for the transaction remains the same. However because the feerate is fetched again, it is possible that it actually isn't and this causes coin selection to fail.
Instead of fetching the feerate each time it is needed, we fetch them all at once at the top of `CreateTransactionInternal`, store them in `CoinSelectionParams`, and use them where needed.
While some of these fee rates probably don't need this caching, I've done it for consistency and the guarantee that they remain the same.
Fixes #19229
ACKs for top commit:
glozow:
reACK https://github.com/bitcoin/bitcoin/commit/f9cd2bfbccb7a2b8ff07cec5f6d2adbeca5f07c3
fjahr:
Code review re-ACK f9cd2bfbccb7a2b8ff07cec5f6d2adbeca5f07c3
Xekyo:
tACK https://github.com/bitcoin/bitcoin/pull/21083/commits/f9cd2bfbccb7a2b8ff07cec5f6d2adbeca5f07c3
meshcollider:
Code review + test run ACK f9cd2bfbccb7a2b8ff07cec5f6d2adbeca5f07c3
Tree-SHA512: be83ff64ba473c3cdd3469c812e214659b6e2a9584c22ed2b1595618fce0d4b35d0901e61068cd1069fc1a8fb911db01dd7312d05c3b8cbafbe2504ab7a3e863
|
|
It's a feerate, not a fee. Also follow the style guide for member names.
|
|
Instead of fetching the discard feerate for each SelectCoinsMinConf
iteration, fetch and cache it once during CreateTransaction so that it
is shared for each SelectCoinsMinConf through
coin_selection_params.m_discard_feerate.
Does not change behavior.
|