fa6b061fc118995eec41766519a11bc0dd1a901d rpc: Auto-format RPCResult (MarcoFalke)
fa7d0503d320900e14c4d9bc016d65c7431070bb rpc: Move OuterType enum to header (MarcoFalke)
Pull request description:
This enforces most syntax rules of the RPCResult at compile time (or some at run time during unit and functional tests)
Apart from normalizing the syntax, by separating stylistic formatting from the structure, we could in theory directly generate the html for e.g. https://bitcoincore.org/en/doc/0.19.0/rpc/wallet/importmulti/
ACKs for top commit:
Sjors:
Indeed, re-ACK fa6b061fc118995eec41766519a11bc0dd1a901d
ajtowns:
ACK fa6b061fc118995eec41766519a11bc0dd1a901d -- skimmed code changes and differences to rpc help output
Tree-SHA512: 5b510b3aa0b7c7b9189a48c77593159409069f939145b9a00c5478e894cf65f994d44d633eb7bb7dbea40ee820645a2930976c24772379d96929002b120efa28
aff2748f8aee72f03b5fb6f6f97f0d0f66391755 httpserver: use own HTTP status codes (Filip Gospodinov)
Pull request description:
Before, macros defined in `<event2/http.h>` have been used for some HTTP status codes. `<event2/http.h>` is included implicitly and the usage of its status code macros is inconsistent with the majority HTTP response implementations in this file.
Now, the `HTTPStatusCode` enum from `<rpc/protocol.h>` is consistently used for all HTTP response implementations.
ACKs for top commit:
practicalswift:
ACK aff2748f8aee72f03b5fb6f6f97f0d0f66391755 -- patch looks correct
laanwj:
ACK aff2748f8aee72f03b5fb6f6f97f0d0f66391755
Tree-SHA512: 6a7043488b88dcd584215d16b5f16f7bd668fe5553d31298d1beef134d2b0648aef81533014e34d1cd600baa36ee4e853f195352f4a00c866bd5ab1ff688bd50
1ef28b4f7cfba410fef524def1dac24bbc4086ca Make AnalyzePSBT next role calculation simple, correct (Gregory Sanders)
Pull request description:
Sniped test and alternative to https://github.com/bitcoin/bitcoin/pull/18220
Sjors documenting the issue:
```
A PSBT signed by ColdCard was analyzed as follows (see #17509 (comment))
{
"inputs": [
{
"has_utxo": true,
"is_final": false,
"next": "finalizer"
}
],
"estimated_vsize": 141,
"estimated_feerate": 1e-05,
"fee": 1.41e-06,
"next": "signer"
}
I changed AnalyzePSBT so that it returns "next": "finalizer" instead.
```
It makes it much clearer that the role has been decided before hitting the `calc_fee` block, and groups all state-deciding in one spot instead of 2.
Note that this assumes that PSBT roles are a complete ordering, which for now and in the future seems to be a correct assumption.
ACKs for top commit:
Sjors:
ACK 1ef28b4f7cfba410fef524def1dac24bbc4086ca, much nicer. Don't forget to document the bug fix.
achow101:
ACK 1ef28b4f7cfba410fef524def1dac24bbc4086ca
Empact:
ACK 1ef28b4f7c
Tree-SHA512: 22ba4234985c6f9c1445b14565c71268cfaa121c4ef000ee3d5117212b09442dee8d46d9701bceddaf355263fe25dfe40def2ef614d4f2fe66c9ce876cb49934
* GetAvoidReuseFlag: simply gets the flag, without modifying the wallet
* ListReceived: helper function to produce lists
* ListTransactions: produces a list of transactions, without modifications; two cases of map [] -> .at() for verified-existing keys
* DescribeWalletAddress: generates a description of a given wallet address without changing the wallet
* The following functions produce a list without making any modifications to the wallet:
* listaddressgroupings
* listreceivedbyaddress
* listreceivedbylabel
* listtransactions
* listsinceblock
* listlockunspent
* listunspent
* listlabels
* getreceivedbyaddress
* getreceivedbylabel
* getaddressesbylabel
* signmessage: uses the wallet to procure a private key for signing, but does no modifications
* getbalance, getunconfirmedbalance: calculates the wallet balance, without any modifications
* gettransaction: procures transaction without any modifications
* backupwallet: makes a backup of the wallet to disk, without changing said wallet
* getwalletinfo: produces info about wallet without any modifications
* signrawtransactionwithwallet: modifies incoming transaction on the fly by signing with private key procured from within wallet; no modifications to wallet
* getaddressinfo: gets information about the given address, with no modifications done to the wallet; one case of [] -> .at() and one ::iterator -> ::const_iterator
* walletprocesspsbt: processes the given PSBT on the fly, without modifying the wallet
* GetWalletAddressesForKey is, as the name implies, immutable; the one change besides the parameter constness is a [] -> .at() change, to a verified-existing key.
* dumpprivkey and dumpwallet are both similarly immutable, for obvious reasons.
CWallet::CanGetAddresses() is used to check whether the wallet has available or is able to produce keys for addresses. It uses the ScriptPubKeyMan::CanGetAddresses(), which in turn uses the const KeypoolCountExternalKeys() method, all which do counting and no modifications.
10efc0487c442bccb0e4a9ac29452af1592a3cf2 Templatize ValidationState instead of subclassing (Jeffrey Czyz)
10e85d4adc9b7dbbda63e00195e0a962f51e4d2c Remove ValidationState's constructor (Jeffrey Czyz)
0aed17ef2892478c28cd660e53223c6dd1dc0187 Refactor FormatStateMessage into ValidationState (Jeffrey Czyz)
Pull request description:
This removes boilerplate code in the subclasses which otherwise only
differ by the result type.
The subclassing was introduced in a27a295.
ACKs for top commit:
MarcoFalke:
ACK 10efc0487c442bccb0e4a9ac29452af1592a3cf2 🐱
ajtowns:
ACK 10efc0487c442bccb0e4a9ac29452af1592a3cf2 -- looks good to me
jonatack:
ACK 10efc048 code review, build/tests green, nice cleanup
Tree-SHA512: 765dd52dde7d49b9a5c6d99d97c96f4492673e2aed0b0604faa88db0308fa4500a26bf755cca0b896be283874096c215932e1110a2d01dc012cd36a5fce58a42
d36146009fb3fc9b9a772823b4df139a85173481 Drop unused mach time headers (Ben Woosley)
Pull request description:
Now that we're no longer special-casing clock usage for MacOS (see #17800), we're
not referencing anything defined in these headers.
Incidentally, this removes our last reference to the `__MACH__` system def. 🎉
ACKs for top commit:
jonasschnelli:
utACK d36146009fb3fc9b9a772823b4df139a85173481
fanquake:
ACK d36146009fb3fc9b9a772823b4df139a85173481 - thanks.
Tree-SHA512: 246045b0683a705ad034416e8ace2024e652026a6c0517b6797320e52fc18a6e111ec2e405ca40653bd1d6421bb7755232e8fec22651fff8e448eb7d5646a954
8888461f6814ae8b6221b02049fb9e1f69a5ff70 util: Fail to parse empty string in ParseMoney (MarcoFalke)
fab30b61eb51538a4db62e34f7133c44575b3fe9 util: Remove unused ParseMoney that takes a c_str (MarcoFalke)
Pull request description:
Supplying a fee rate or an amount on the command line as an empty string, which currently parses as `0` seems fragile and confusing. See for example the confusion in #18214.
Fixes #18214
ACKs for top commit:
Empact:
Code Review ACK 8888461f68
achow101:
ACK 8888461f6814ae8b6221b02049fb9e1f69a5ff70
instagibbs:
utACK 8888461f68
Tree-SHA512: ac2d6b7fa89fe5809c34d5f49831042032591c34fb3c76908d72fed51e8bced41bf2b41dc1b3be34ee691a40463355649857a7a8f378709d38ae89503feb11c2
16d6113f4faa901e248adb693d4768a9e5019a16 Refactor message transport packaging (Jonas Schnelli)
Pull request description:
This PR factors out transport packaging logic from `CConnman::PushMessage()`.
It's similar to #16202 (where we refactor deserialization).
This allows implementing a new message transport protocol like BIP324.
ACKs for top commit:
dongcarl:
ACK 16d6113f4faa901e248adb693d4768a9e5019a16 FWIW
ariard:
Code review ACK 16d6113
elichai:
semiACK 16d6113f4faa901e248adb693d4768a9e5019a16 ran functional+unit tests.
MarcoFalke:
ACK 16d6113f4faa901e248adb693d4768a9e5019a16 🙎
Tree-SHA512: 8c2f8ab9f52e9b94327973ae15019a08109d5d9f9247492703a842827c5b5d634fc0411759e0bb316d824c586614b0220c2006410851933613bc143e58f7e6c1
dc9305b6162ec615ff5fb2876e4f312051b543af random: don't special case clock usage on macOS (fanquake)
Pull request description:
`clock_gettime()`, `CLOCK_MONOTONIC` and `CLOCK_REALTIME` are all available for use on
macOS (now that we require macOS >=10.12 and build against 10.14). Use them rather than the [deprecated](https://developer.apple.com/library/archive/documentation/Darwin/Conceptual/KernelProgramming/Mach/Mach.html) `mach_timespec_t` time API.
I mentioned the possibility for this change [in #17270](https://github.com/bitcoin/bitcoin/pull/17270#discussion_r346090606).
[master](1dbf3350c683f93d7fc9b861400724f6fd2b2f1d):
```bash
2019-12-23T20:49:43Z Feeding 216 bytes of dynamic environment data into RNG
2019-12-23T20:50:43Z Feeding 216 bytes of dynamic environment data into RNG
```
This PR:
```bash
2019-12-23T20:32:41Z Feeding 232 bytes of dynamic environment data into RNG
2019-12-23T20:33:42Z Feeding 232 bytes of dynamic environment data into RNG
```
~~Depends on #16392.~~ Merged.
ACKs for top commit:
laanwj:
ACK dc9305b6162ec615ff5fb2876e4f312051b543af
Tree-SHA512: 18c2f336ea628f9cf7339b817381d230a18893fd9c0351bf99a39ca6f45c5b0a20af9d599d48d6c09515627d5edafa91337c17f9f790264251d2cdcb3763bbd5
7bf4ce4f644bb7dac9b63172c656b5d599eedea3 refactor: test/bench: dedup SetupDummyInputs() (Sebastian Falbesoner)
Pull request description:
The only difference between `SetupDummyInputs()` in `test/transaction_tests.cpp` and the one in `bench/ccoins_caching.cpp` was the nValue amounts of the outputs, so we allow to pass those in an extra (fixed-size) array parameter.
ACKs for top commit:
MarcoFalke:
re-ACK 7bf4ce4f64, only change is schuffling includes 🚶
Empact:
ACK 7bf4ce4f64
Tree-SHA512: e13643b2470f6b6ab429da0c0a8eebd4cb41e2ff2e421ef36f85fa4847bf4ea8aab88d59a01e94cac4c4eb85edb561463f02215b174c50b573ac6bbcc2bf98a3
The function IsStandardTx() returns rejection reason "scriptsig-not-pushonly"
if the transaction has at least one input for which the scriptSig consists of
any other ops than just PUSHs.
The only difference between SetupDummyInputs() in test/transaction_tests.cpp
and the one in bench/ccoins_caching.cpp was the nValue amounts of the outputs,
so we allow to pass those in an extra (fixed-size) array parameter.
c72a11a1a030036eb1fe4472086a9733731961ce test: Add cost_of_change parameter assertions to bnb_search_test (Yancy Ribbens)
Pull request description:
If the `cost_of_change` variable is removed from the method body `SelectCoinsBnB`, there are currently no failing unit tests. This PR adds assertions about the behavior of the `cost_of_change`: If the cost of creating a change output is greater than what's leftover, then consume the output and create no change, otherwise, don't consume the output (no match found).
ACKs for top commit:
achow101:
ACK c72a11a1a030036eb1fe4472086a9733731961ce
Tree-SHA512: 613aa411df5e2911446e0e8bf3309336faaadf2d3c56e7d125b76454e7c6f9e4f5e8f0910dc6222282628e38cd8a4a7c56bb3d36b564a17f396b9b503ecc64c8
fab2527515e8db944ae044bea8580e2cd9414bcd test: Disable mockforward scheduler unit test for now (MarcoFalke)
Pull request description:
This should be a workaround to fix #18174 in the short run and buy us more time to investigate the issue while ci runs are green again 🙏
ACKs for top commit:
fanquake:
ACK fab2527515e8db944ae044bea8580e2cd9414bcd - be good to get Travis back.
laanwj:
ACK fab2527515e8db944ae044bea8580e2cd9414bcd
Tree-SHA512: 027e86b3dfec203a464e5bf528e9933c208c36633c2d4bfcdbc10da1799637a5d6ea0a63af33a4174fb1ad7115df631a4cb838f56e31f4cbd15498e1e9fdf9cc
0653939ac130eddffe40c53ac418bea305d3bf82 Add static_asserts to ser_X_to_Y() methods (Samer Afach)
be94096dfb0c4862e2314cbae4120d7360b08ef2 Fix a violation of C++ standard rules that unions cannot be switched. (Samer Afach)
Pull request description:
Type punning in C++ is not like C. As per the C++ standard, one cannot use unions to convert the bit type. A discussion about this can be found [here](https://stackoverflow.com/questions/25664848/unions-and-type-punning). In C++, a union is supposed to only hold one type at a time. It's intended to be used only as `std::variant`. Switching types is undefined behavior.
In fact, C++20 has a special casting function, called [`bit_cast`](https://en.cppreference.com/w/cpp/numeric/bit_cast) that solved this problem.
Why has it been working so far? Because some compilers tolerate using unions and switching types, like gcc. More information [here](https://gcc.gnu.org/onlinedocs/gcc/Optimize-Options.html#Type-punning).
One important thing to mention is that performance is generally not affected by that memcpy. Compilers are smart enough to convert that to a memory cast when possible. But we have to do it the right way, otherwise, it's jut undefined behavior that depends on the compiler.
ACKs for top commit:
practicalswift:
ACK 0653939ac130eddffe40c53ac418bea305d3bf82
elichai:
ACK 0653939ac130eddffe40c53ac418bea305d3bf82
laanwj:
Code review ACK 0653939ac130eddffe40c53ac418bea305d3bf82
kristapsk:
ACK 0653939ac130eddffe40c53ac418bea305d3bf82
Tree-SHA512: f6e89de39fc964750429139bab6b5a1346f7060334b7afa020e315bdad8f8c195bce2b8a9e343f06e7fff175e2dfb1cdabfcb6fe405bea0febe4962f0cc62557
facb71576cd4d2e90fd03e09d29b42fa3d730e8c net: Remove forcerelay of rejected txs (MarcoFalke)
Pull request description:
This removes the code that supposedly handled the forced relay of txs from a permissioned peer that were rejected from our mempool. The removal should be fine, because it is dead code for the following reasons:
* While `RelayTransaction` enqueues the inv for all peers, the inv is never processed because it can not be found in the mempool. See 4a07233076/src/net_processing.cpp (L3862-L3866)
* Even if the peers we intended to send the inv to can somehow reply with a getdata to the never-received inv, they won't receive the tx as a reply because it was never added to the "relay memory" (`mapRelay`)
The dead code is (obviously) untested: https://marcofalke.github.io/btc_cov/total.coverage/src/net_processing.cpp.gcov.html#2574
This feature was (intentionally or accidentally) removed in 4d8993b3469915d8c9ba4cd3b918f16782edf0de, which was released in Bitcoin Core 0.13.0. So all currently supported versions of Bitcoin Core ship without this feature. I am not aware of any complaints about this feature or actual documented use-cases. So instead of reviving an unneeded feature, just remove the dead code.
ACKs for top commit:
hebasto:
ACK facb71576cd4d2e90fd03e09d29b42fa3d730e8c, locally running the unit and functional tests.
Tree-SHA512: bfceae6f2983c1510fa0649a9a63c343cbbc1c4ab3a3698039cccf454c81e58c8f5114b147ed42a1bc867da74c43a5b53764ab14f942e191b6f59079044108b5
This is needed so that it can be used by RPCResult
Also,
* rename NAMED_ARG to NONE for generalization.
* change RPCArg constructors to initialize the members by moving values
5bad7921d0b33b62c0a59a478c2e8c869fc5e3b5 [test] PSBT RPC: check that bip32_derivs are present by default (Sjors Provoost)
29a21c90610aed88b796a7a5900e42e9048b990e [rpc] set default bip32derivs to true for psbt methods (Sjors Provoost)
Pull request description:
In https://github.com/bitcoin/bitcoin/pull/13557#pullrequestreview-135905054 I recommended not including bip32 deriviation by default in PSBTs:
> _Bit of a privacy issue_: let's say person A and B are about to spend from a multisig address, sending everything to person A. Person A gives their address to person B, their wallet wallet creates a PSBT, but doesn't sign it. Wallet A then calls `walletprocesspsbt` which signs it and _spontaneously adds the master_fingerprint and bip32 path_. Same issue with `walletcreatefundedpsbt`.
>
> Adding `bip32_derivs` should probably be opt-in.
In practice I find this default quite annoying because I forget it and end up with a confused hardware wallet.
More importantly, in the multisig example I provided, it's actually essential for the other side to know the derivation details (in addition to an xpub). This allows them to check that change is going to an address you can still co-sign for (because the spending policy is unchanged except for an index).
ACKs for top commit:
instagibbs:
utACK 5bad7921d0
jonatack:
ACK 5bad7921d0 code review, built, ran tests, inspected/messed around with/pprinted values from the new tests. Thanks for adding the tests.
meshcollider:
utACK 5bad7921d0b33b62c0a59a478c2e8c869fc5e3b5
Tree-SHA512: 22ad71dda96856060a96758c4ae7aafa22d5e9efba30e0c8287c711e7579849bd72593cbc0f41a2e9e8821315d78bda04e848dbb006283b841b2795e2faebcfd
e193a84fb28068e38d5f54fbfd6208428c5bb655 Refactor message hashing into a utility function (Jeffrey Czyz)
f8f0d9893d7969bdaa870fadb94ec5d0dfa8334d Deduplicate the message signing code (Vasil Dimov)
2ce3447eb1e25ec7aec4b300dabf6c1e394f1906 Deduplicate the message verifying code (Vasil Dimov)
Pull request description:
The message signing and verifying logic was replicated in a few places
in the code. Consolidate in a newly introduced `MessageSign()` and
`MessageVerify()` and add unit tests for them.
ACKs for top commit:
Sjors:
re-ACK e193a84fb28068e38d5f54fbfd6208428c5bb655
achow101:
ACK e193a84fb28068e38d5f54fbfd6208428c5bb655
instagibbs:
utACK e193a84fb2
meshcollider:
utACK e193a84fb28068e38d5f54fbfd6208428c5bb655
Tree-SHA512: b0e02a7d4623a98c8f8c77627af1725e6df07700de4630c2f75da6beacdf55414c38ba147bc6d2a757491ab07c827dddf93e8632fe600478760e255714ddab88
