b6aadcd5b4350a6ebcd57e88e7a0853cedf7c2fb build: Add -Werror=mismatched-tags (Hennadii Stepanov)
1485124291368c4a2ca8ea09c18e813f1dbabf5c Fix -Wmismatched-tags warnings (Hennadii Stepanov)
Pull request description:
Warnings were introduced in #20749:
```
./validation.h:43:1: warning: class 'CCheckpointData' was previously declared as a struct; this is valid, but may result in linker errors under the Microsoft C++ ABI [-Wmismatched-tags]
class CCheckpointData;
^
./chainparams.h:24:8: note: previous use is here
struct CCheckpointData {
^
./validation.h:43:1: note: did you mean struct here?
class CCheckpointData;
^~~~~
struct
1 warning generated.
```
This change fixes AppVeyor build: https://ci.appveyor.com/project/DrahtBot/bitcoin/builds/37547435
ACKs for top commit:
glozow:
utACK b6aadcd5b4🚗
practicalswift:
cr ACK b6aadcd5b4350a6ebcd57e88e7a0853cedf7c2fb: patch looks correct
Tree-SHA512: 3ac887ebdbf9a1ae33c1fd5381b3b8d83388ad557ddeb55013acd42bb9752a5bd009e3a0eed52644a023a7a0dda1c159277981af82f58fb0abfe60b84e01bf29
5b41d84b3469484ff6a1f4eb7c890b6444a16091 doc: add xorriso to macOS depends packages (fanquake)
Pull request description:
This was missed in #20470.
ACKs for top commit:
hebasto:
ACK 5b41d84b3469484ff6a1f4eb7c890b6444a16091, tested on Linux Mint 20.1 (x86_64).
Tree-SHA512: bcfd8468a099c69175f8a9d295c1466764ab25d6a61121b28675a09c3e96f45b6309e1523d341f4cb21d0ddee4945f00ba060ba02da835f2f0db7e694fd6c44b
ad57fb756b1c2df625790bd9c296ec28daa93740 wallet: Add BerkeleyDB version sanity check at init time (Wladimir J. van der Laan)
Pull request description:
Detect version conflicts between the run-time BerkeleyDB library and the one used during compilation.
This is very unsafe (can result in anything from crashes to corruption) so shut down when one is detected.
ACKs for top commit:
decryp2kanon:
utACK ad57fb7
achow101:
ACK ad57fb756b1c2df625790bd9c296ec28daa93740
theStack:
utACK ad57fb756b1c2df625790bd9c296ec28daa93740
meshcollider:
Code review ACK ad57fb756b1c2df625790bd9c296ec28daa93740
Tree-SHA512: 99cd7d836bffbdeb3d4e14053f7139cc85a6d42e631a3f9a3058a848042446b364faee127500f5acb374616e6a61ab2bedebfac1ba9bc993b4d6227114c2a6c2
faccf8b1e1af293dfe9158d732718e7798a2fd89 refactor: Enable -Wswitch for FeeEstimateHorizon (MarcoFalke)
Pull request description:
This enables the `-Wswitch` compiler warning for `FeeEstimateHorizon` by removing the `default` case in `switch` statements.
ACKs for top commit:
practicalswift:
cr ACK faccf8b1e1af293dfe9158d732718e7798a2fd89
jonatack:
ACK faccf8b1e1af293dfe9158d732718e7798a2fd89
hebasto:
ACK faccf8b1e1af293dfe9158d732718e7798a2fd89, I have reviewed the code and it looks OK, I agree it can be merged.
Tree-SHA512: 63a8dff6e8dead149ec2fa8319e7ff41022c9534d423d3086fd8f22be073dc4915f74c7fe9139ee681a8204730cf58c80ef40c93fb33032d586e68b4f78f557d
fa0d8359b351fd179a0a2f458671a4d7828c9a80 log: Clarify that failure to read fee_estimates.dat is non-fatal (MarcoFalke)
faefa5db5f1d95b772873f4429e8a8fbb4e71cf3 log: Clarify that failure to write fee_estimates.dat is non-fatal (MarcoFalke)
Pull request description:
two minor logging fixups
ACKs for top commit:
practicalswift:
ACK fa0d8359b351fd179a0a2f458671a4d7828c9a80: patch looks correct
laanwj:
Code review ACK fa0d8359b351fd179a0a2f458671a4d7828c9a80
Tree-SHA512: d1e7e595d3b4a5e497ee7ab70f3be5783dafec2726ef8e012db836c15e8e622022859a4472d6b516fe19d327737b25fdfb509cd9aeb022ca847b13c54e55800a
fa6af312277bb1b7e57d9b764d411c5b0873829f test: Document why syncwithvalidationinterfacequeue is needed in tests (MarcoFalke)
fa135a13b8ddaa117bd090ec43a3eab3a95755c1 Revert "test: Add missing sync_all to wallet_balance test" (MarcoFalke)
Pull request description:
syncwithvalidationinterfacequeue is a hidden test-only RPC, so it should not be used when it is not needed. Thus, either remove it or explain why it is needed.
ACKs for top commit:
fjahr:
Code review ACK fa6af312277bb1b7e57d9b764d411c5b0873829f
Tree-SHA512: de30db4ab521184091ee5beeab02989138cf7cf05088f766a2fb106151b239310b63d5380cb79e2a072f72c5ae9513aecae8eb9c1c7be713771585c3cb04d63a
3ebde2143aa98af213872b98b474d904e55056f7 [test] Fix wait condition in disconnect_p2ps (Amiti Uttarwar)
Pull request description:
#19315 currently has a [test failure](https://cirrus-ci.com/task/4545582645641216) because of a race. `disconnect_p2ps` is intended to have a `wait_until` clause that prevents this race, but the conditional doesn't match since its comparing two different object types. `MY_SUBVERSION` is defined in messages.py as a byte string, but is compared to the value returned by the RPC. This PR simply converts types to ensure they match, which should prevent the race from occurring.
HUGE PROPS TO jnewbery for discovering the issue 🔎
ACKs for top commit:
jnewbery:
ACK 3ebde2143aa98af213872b98b474d904e55056f7
glozow:
Code review ACK 3ebde2143a
Tree-SHA512: ca096b80a3e4d757a645f38846d6dc89d6a3d35c3435513a72d278e305faddd4aff9e75a767941b51b2abbf59c82679bac1e9a0140d6f285efe3053e51bcc2a8
defe48a51f4315f8cc607875a099981593c8cc39 doc: Update wallet files in files.md (Hennadii Stepanov)
Pull request description:
This PR is a #19077 follow up, and it addresses the [comment](https://github.com/bitcoin/bitcoin/pull/19077#discussion_r504805234):
> If need to update, there are two corrections that could be made:
>
> * Line 69 "Wallets are Berkeley DB (BDB) databases" is no longer true
>
> * Line 76 "Wallet lock file" should say "BDB wallet lock file"
ACKs for top commit:
RiccardoMasutti:
ACK defe48a
meshcollider:
ACK defe48a51f4315f8cc607875a099981593c8cc39
Tree-SHA512: 39939f86a9c7842bf06913998305dcbd6209585f1da0fe9c274bac0572eb8464e59176884dd9e2b91312f34efad40cdeb4085ec72c2a2c1b33d16b6ab505140c
675e55e01392971aa56bda56cb09498b466d0902 Ignore unknown messages before VERACK (Suhas Daftuar)
Pull request description:
This allows for feature negotiation to take place with messages between VERSION and VERACK in the future, without requiring additional software changes to specifically ignore messages for features that are unimplemented by our software.
ACKs for top commit:
sipa:
utACK 675e55e01392971aa56bda56cb09498b466d0902
practicalswift:
ACK 675e55e01392971aa56bda56cb09498b466d0902: patch looks correct
MarcoFalke:
ACK 675e55e01392971aa56bda56cb09498b466d0902
hebasto:
ACK 675e55e01392971aa56bda56cb09498b466d0902, the offender peer will be eventually disconnected due to the timeout.
Tree-SHA512: 8d2b1d8b9843f2ee26b2c30f7c5ff0bfcfbe3f46b32cd0369c48ece26624151091237e83ce3f18c6da004099026602cfab1642ac916db777f047d170b365c007
638441928a446726ce3a7fb20433a5478e7585bb test: add parameterized constructor for msg_sendcmpct() (Sebastian Falbesoner)
Pull request description:
While working on the test for #19776 I noticed that creating a `sendcmpct` message is quite cumbersome -- due to the lack of a parameterized constructor, one needs to create an empty (that is, initialized with default values) object and then set the two fields one by one. This PR replaces the default constructor with a parameterized constructor and uses it in the test `p2p_compactblocks.py`, reducing LOC. No need to pollute the namespace with temporary throw-away message objects anymore.
ACKs for top commit:
guggero:
Code review ACK 638441928a446726ce3a7fb20433a5478e7585bb.
epson121:
Code review ACK 638441928a446726ce3a7fb20433a5478e7585bb
Tree-SHA512: 3b58d276d714b73abc6cc98d1d52dec5f6026b33f03faaeb7dcbc5d83ac377555179f98b159b2b9ecc8957999c35a1dc082e3c69299c5fde4e35f1bd0587ce9d
637d8bce741213295bd9b9d1982cae663c701ba1 Change FILE_CHAR_BLOCKLIST to FILE_CHARS_DISALLOWED (Benoit Verret)
Pull request description:
Blocklist is ambiguous. It could mean a list of blocks.
Example: "blocknotify" in the same file refers to Bitcoin blocks.
ACKs for top commit:
MarcoFalke:
ACK 637d8bce741213295bd9b9d1982cae663c701ba1
laanwj:
ACK 637d8bce741213295bd9b9d1982cae663c701ba1 — this is a clear variable name improvement
theStack:
ACK 637d8bce741213295bd9b9d1982cae663c701ba1
jonatack:
ACK 637d8bce741213295bd9b9d1982cae663c701ba1
eriknylund:
ACK 637d8bce741213295bd9b9d1982cae663c701ba1
promag:
ACK 637d8bce741213295bd9b9d1982cae663c701ba1.
Tree-SHA512: 028e7102eeaf61105736c55c119a7f5c05411f2b6715a7939c41cb9e8f13afb757bbb6e7a302b3aae21722e69dab91f6eff8099e5884d248299905b4c7687c02
6cef3652d143a1dddad1254cab0953561d24c1fa build: fix -Wformat-security check when compiling with GCC (fanquake)
Pull request description:
GCC expects `-Wformat` to be passed with [`-Wformat-security`](https://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html), which means
when we test for it in configure it currently fails:
```bash
checking whether C++ compiler accepts -Wformat-security... no
...
configure:15907: checking whether C++ compiler accepts -Wformat-security
configure:15926: g++ -std=c++11 -c -g -O2 -Werror -Wformat-security conftest.cpp >&5
cc1plus: error: '-Wformat-security' ignored without '-Wformat' [-Werror=format-security]
cc1plus: all warnings being treated as errors
```
and never gets added to our CXX flags. Note that Clang does not have this requirement and the check is working correctly there.
The change in this PR is the simple fix, however we might want to consider using something like `-Wformat=2` in future, which in GCC is equivalent to `-Wformat -Wformat-nonliteral -Wformat-security -Wformat-y2k.` and similar [in Clang](https://clang.llvm.org/docs/DiagnosticsReference.html#wformat-2).
ACKs for top commit:
practicalswift:
ACK 6cef3652d143a1dddad1254cab0953561d24c1fa
laanwj:
ACK 6cef3652d143a1dddad1254cab0953561d24c1fa
Tree-SHA512: f9230d42af39f85ea9d2f55dbbebd2bae4740fe59b0da2e092af3ac9ef7e6799d3a4cf83eb64574c63982e5f6b14e226d44c84fa0335255d65c9947d86a1ea38
854382885f18aa9a95cdde3d11591b05c305ad3f refactor: test: improve wait_for{header,merkleblock} interface (Sebastian Falbesoner)
1356a45ef042e7bd3d539fbb606d6b1be547d00f test: complete impl. of msg_merkleblock and wait_for_merkleblock (Sebastian Falbesoner)
Pull request description:
Implements the missing initialization/serialization methods for `msg_merkleblock`, based on the already present class `CMerkleBlock`. Also changes the method `wait_for_merkleblock()` to be more precise by waiting for a merkleblock with a specified blockhash instead of an arbitrary one.
In the BIP37 test `p2p_filter.py`, this new method is used to make the test of receiving merkleblock and tx if a filter is set to be more precise, by checking if they also arrive in the right order.
In the course of this PR, also the interface for the methods `wait_for_merkleblock()` and `wait_for_header()` are improved to take a hex string instead of an integer, which is more typesafe and less of a burden to the caller.
ACKs for top commit:
MarcoFalke:
ACK 854382885f18aa9a95cdde3d11591b05c305ad3f
Tree-SHA512: adaf0ac728ef0b9929cb417a7a7b4c1346c400b2d365bf6914515c67b6cfe8f4a7ecc62fb514afdce9792f0bed833416f6bca6b9620f3d5dcdc66e4d5b0b7ea3
e1e1442f3eadc1d139380e71c1b60b86d8d6bdee Activate no-privkey -> ISMINE_WATCH_ONLY behavior for LegacySPKM only (Gregory Sanders)
Pull request description:
Slight cleanup following https://github.com/bitcoin/bitcoin/pull/16944
This should allow future scriptpubkeymans to transparently work, since the current plan is to have ismine always be spendable.
ACKs for top commit:
achow101:
ACK e1e1442f3eadc1d139380e71c1b60b86d8d6bdee
Sjors:
Code review ACK e1e1442f3eadc1d139380e71c1b60b86d8d6bdee
meshcollider:
Code review ACK e1e1442f3eadc1d139380e71c1b60b86d8d6bdee
Tree-SHA512: c0a86587d33b8b1646494a5cb0bf8681ee4a88e6913918157746943a0996b501903e0e6ee954cf04154c1e0faee0cbb375c74ca789f46ba9244eb5296632b042
## Issue being fixed or feature implemented
https://github.com/dashpay/dash/issues/5294
## What was done?
Called `contrib/devtools/gen-manpages.sh` on top of new assume valid
values, see dash#5304
## How Has This Been Tested?
Reviewed only
## Breaking Changes
No breaking changes
## Checklist:
- [x] I have performed a self-review of my own code
- [x] I have assigned this pull request to a milestone
## Issue being fixed or feature implemented
1. we need to move time forward to let invs being relayed
2. nNextInvSend in SendMessages can be bumped up to 30+ seconds into the
future in rare cases
make sure timeouts in tests are high enough to relay tx inv/wait for
corresponding islock
## What was done?
tl;dr: bump mocktime while waiting, wait longer
extracted fixes from https://github.com/dashpay/dash/pull/5288 but I
expect this to fix other sporadic test failures too
## How Has This Been Tested?
tests are ok locally and in https://github.com/dashpay/dash/pull/5288
## Breaking Changes
n/a
## Checklist:
- [x] I have performed a self-review of my own code
- [ ] I have commented my code, particularly in hard-to-understand areas
- [ ] I have added or updated relevant unit/integration/functional/e2e
tests
- [ ] I have made corresponding changes to the documentation
**For repository code-owners and collaborators only**
- [ ] I have assigned this pull request to a milestone
## Motivation
Dash Core has a series of functional tests that do not behave in a
deterministic fashion and have a higher chance of failure, especially on
resource limited systems. This results in a lot of false-negatives
during CI runs, prompting the need to re-run them, which is annoying at
best and generates apathy towards CI failure at worst.
## History
The first approach was to isolate non-deterministic tests into their own
distinct GItLab runner, making it such that if a test failed, only that
one runner had to be restarted, instead of the multiple runners that
failed due to these tests.
One problem with this was that this approach effectively omitted these
tests from TSan and UBSan coverage as attempting to combine TSan and
UBSan would cause significant resource exhaustion.
## Description
An alternative approach is to introduce a new flag, `--retries`,
applicable only on non-deterministic tests, that allow a failed test to
be repeated up to a set number of times (default: 3), only reporting
failure once the limit is exhausted.
A limitation of this is that only the log dump from the last attempt
will be available.
---------
Co-authored-by: UdjinM6 <UdjinM6@users.noreply.github.com>
This backports also includes extra changes that were missing from bitcoin#14711
They are required, otherwise impossible to remove validaion.h dependency as it meant in #17138
b96ed0396294fc4fa89d83ceab6bc169dd09f002 [wallet] Remove pruning check for -rescan option (John Newbery)
eea462de9c652dca556ad241d2126b10790f67f8 [wallet] Remove package limit config access from wallet (John Newbery)
Pull request description:
Removes wallet access to `-limitancestorcount`, `-limitdescendantcount` and `-prune`:
- `-limitancestorcount` and `-limitdescendantcount` are now accessed with a method `getPackageLimits` in the `Chain` interface.
- `-prune` is not required. It was only used in wallet component initiation to prevent running `-rescan` when pruning was enabled. This check is not required.
Partially addresses #17137.
ACKs for top commit:
MarcoFalke:
Tested ACK b96ed0396294fc4fa89d83ceab6bc169dd09f002
ryanofsky:
Code review ACK b96ed0396294fc4fa89d83ceab6bc169dd09f002
promag:
Code review ACK b96ed0396294fc4fa89d83ceab6bc169dd09f002.
ariard:
ACK b96ed03, check there isn't left anymore wallet access to node arguments.
Tree-SHA512: 90c8e3e083acbd37724f1bccf63dab642cf9ae95cc5e684872a67443ae048b4fdbf57b52ea47c5a1da6489fd277278fe2d9bbe95e17f3d4965a1a0fbdeb815bf
- updated data file with dash addresses
- removed witness/bench32 support from rpc_createmultisig.py
- other specific changes, such as wallet balances after N mined blocks
- updated descriptors for multisort sign (fixes for bitcoin#17056)
19a354b11f85a3c6c81ff83bf702bf7a40cf5046 Output a descriptor in createmultisig and addmultisigaddress (Andrew Chow)
Pull request description:
Give a descriptor from `createmultisig` and `addmultisigaddress`.
Extracted from #16528 with `addmultisgaddress` and tests added.
ACKs for top commit:
Sjors:
tACK 19a354b11f85a3c6c81ff83bf702bf7a40cf5046
MarcoFalke:
ACK 19a354b11f85a3c6c81ff83bf702bf7a40cf5046
promag:
Code review ACK 19a354b11f85a3c6c81ff83bf702bf7a40cf5046.
meshcollider:
utACK 19a354b11f85a3c6c81ff83bf702bf7a40cf5046
Tree-SHA512: e813125fbbc358ea8d45b1748de16a29a94efd83175b748fb8fa3b0bfc8e783ed36b6c554d84f5d4ead1ba252a83a3e937b6c3f75da7b8d3b4e55f94d6013771
4bb660be90a2811b53855bf1fd33a8dd9ba3db47 Add release note (Andrew Chow)
ed96b295d747738334459490c79b7360ab85aaf7 Update descriptors.md to include sortedmulti (Andrew Chow)
80be78ea75ac9833ee3db3d468ed09fc4fe6274c Test sortedmulti descriptor using BIP 67 tests (Andrew Chow)
6f588fd2276e5b713c6d36e3b01288484ddb59c0 Add sortedmulti descriptor and unit tests (Andrew Chow)
Pull request description:
Adds a `sortedmulti()` descriptor as mentioned in https://github.com/bitcoin/bitcoin/pull/17023#issuecomment-537596416.
`sortedmulti()` works in the same way as `multi` does but sorts the pubkeys in the resulting scripts in lexicographic order as described in [BIP67](https://github.com/bitcoin/bips/blob/master/bip-0067.mediawiki). Note that this does not add support for BIP67 nor is BIP67 fully supported by this descriptor (which is why it is not named `multi67()`) as it does not require compressed pubkeys.
Tests from BIP67 were added and documentation was updated.
ACKs for top commit:
instagibbs:
re-ACK 4bb660be90
Sjors:
re-ACK 4bb660be90a2811b53855bf1fd33a8dd9ba3db47
Tree-SHA512: 93b21112a74ebe0bf316d8f3e0291f69fd975cf0a29332f9728e7b880cad312b8b14007e86adcd7899f117b9303cbcf4cb35f3bb2f2f648d1a446f83f75a70a5
ec4c79326bb670c2cc1757ecfb1900f8460c5257 signrawtransaction*: improve error for partial signing (Anthony Towns)
3c481f8921bbc587cf287329f39243abe703b868 signrawtransactionwithkey: better error messages for bad redeemScript/witnessScript (Anthony Towns)
Pull request description:
Two fixes for `signrawtransactionwith{key,wallet}` (in addition to #16250): one that checks redeemScript/witnessScript matches scriptPubKey (and if both are provided that they match each other sanely), and the other changes the warning when some-but-not-all the signatures for a CHECKMULTISIG are provided to something that suggests more signatures may be all that's required.
Fixes: #13218Fixes: #14823
ACKs for top commit:
instagibbs:
utACK ec4c79326b
achow101:
Code Review ACK ec4c79326bb670c2cc1757ecfb1900f8460c5257
meshcollider:
utACK ec4c79326bb670c2cc1757ecfb1900f8460c5257
Tree-SHA512: 0c95c91d498e85b834662b9e5c83f336ed5fd306be7701ce1dbfa0836fbeb448a267a796585512f7496e820be668b07c2a0a2f45e52dc23f09ee7d9c87e42b35
01174596e69568c434198a86f54cb9ea6740e6c2 signrawtransactionwithkey: report error when missing redeemScript/witnessScript param (Anthony Towns)
Pull request description:
Adding support for "witnessScript" as an alternative to "redeemScript" when using "signrawtransactionwithkey" meant that the `RPCTypeCheckObj()` call in `SignTransaction` can't error out just because either parameter is missing -- it's only a problem if both are missing, which isn't a state `RPCTypeCheckObj()` tests for. This results in the regression described in #16249. This patch adds some code to test for this case and give a similar error, namely:
error code: -8
error message:
Missing redeemScript/witnessScript
Fixes: #16249
ACKs for top commit:
meshcollider:
utACK 01174596e6
promag:
ACK 01174596e. Could also write test without `dict`/`del`:
Tree-SHA512: cf51346b7dea551b7f18f2a93c2a336a293b2535c62c03a5263cd2be8c58cf0cc302891da659c167e88ad1a68a756472c3c07e99f71627c61d32886fc5a3a353
fab6a0a659 test: Add test that addmultisigaddress fails for watchonly addresses (MarcoFalke)
fad81d870a test: Fixup creatmultisig documentation and whitespace (MarcoFalke)
Pull request description:
Just to make sure this is not regressed on accidentally in the future
ACKs for commit fab6a0:
jonatack:
ACK fab6a0a659
Tree-SHA512: bf8dcbc752f8910902a995e55ce486621156aa01f112990344815c4aab980298dfecc108e78245a8986a00c3871338ad16fc818a1bce9dfc6b37b9c88851e39d
f40b3b82df [tests] functional test for createmultisig RPC (Anthony Towns)
b9024fdda3 segwit support for createmultisig RPC (Anthony Towns)
d58055d25f Move AddAndGetDestinationForScript from wallet to outputype module (Anthony Towns)
9a44db2e46 Add outputtype module (Anthony Towns)
Pull request description:
Adds an "address_type" parameter that accepts "legacy", "p2sh-segwit", and "bech32" to choose the type of address created. Defaults to "legacy" rather than the value of the `-address-type` option for backwards compatibility.
As part of implementing this, OutputType is moved from wallet into its own module, and `AddAndGetDestinationForScript` is changed to apply to a `CKeyStore` rather than a wallet, and to invoke `keystore.AddCScript(script)` itself rather than expecting the caller to have done that.
Fixes#12502
Tree-SHA512: a08c1cfa89976e4fd7d29caa90919ebd34a446354d17abb862e99f2ee60ed9bc19d8a21a18547c51dc3812cb9fbed86af0bef2f1e971f62bf95cade4a7d86237
eeeaa29214 descriptors.md: Refer to descriptors as describing instead of matching (Russell Yanofsky)
eb49412562 doc/descriptors.md tweaks (Russell Yanofsky)
Pull request description:
Add some implementation details, and tweak phrasing in examples section to be more explicit about how expressions are used for matching.
Tree-SHA512: a9dc7bc0fc370548189a789f31c04bd11103cdd2a99bcb909fa1b1dfa4e78509813dad5d5c9e3db98d66929f45cb5704f5c46ab4cbd800fef22cd8465f80ef33
fa1ad8f06eba5e120c30f07263250bc382891179 build: Bump gitian descriptor versions (MarcoFalke)
Pull request description:
Bump the gitian descriptor versions as a follow-up to #17007.
Also fixes#17027 with a cherry-pick, and bump the manpages.
ACKs for top commit:
fanquake:
ACK fa1ad8f06eba5e120c30f07263250bc382891179
Tree-SHA512: c3b669c3797e5febb51a8dd01e2621a7544a291e080d73c47a2a12ea9da84ff904533e68792e2e869ebbdc2226b2fee7517214549e6cc7e988f175098f7c412c
1b41c2c8a126ef4be183e1d800a17d85cab8837b test: improve gettransaction test coverage (Jon Atack)
0f34f54888f680bfbe7a29ac278636d7178a99bb rpc: fix regression in gettransaction (Jon Atack)
Pull request description:
Closes#16872.
PR #16866 renamed the `decode` argument in gettransaction to `verbose` to make it more consistent with other RPC calls like getrawtransaction. However, it inadvertently overloaded the "details" field when `verbose` is passed. The result is that the original "details" field is no longer returned correctly, which seems to be a breaking API change.
This PR:
- takes the simplest path to restoring the "details" field by renaming the decoded one back to "decoded" while leaving the `verbose` argument for API consistency, which was the main intent of #16866,
- addresses [this comment](https://github.com/bitcoin/bitcoin/pull/16185#discussion_r320740413) by mentioning in the RPC help that the new decoded field is equivalent to decoderawtransaction, and
- updates the help, functional test, and release note.
Reviewers, to test this manually, build and run `bitcoin-cli help gettransaction` and `bitcoin-cli gettransaction <wallet txid> false true`, and verify that the command returns both `details` and `decoded` fields.
ACKs for top commit:
jnewbery:
tACK 1b41c2c8a126ef4be183e1d800a17d85cab8837b
Tree-SHA512: 287edd5db7ed58fe8b548975aba58628bd45ed708b28f40174f10a35a455d89f796fbf27430aa881fc376f47aabda8803f74d4d100683bd86577a02279091cf3
7dee8f48088c75ab0e51be60679505f8ce570919 [wallet] Rename 'decode' argument in gettransaction method to 'verbose' (John Newbery)
Pull request description:
This makes the RPC method consistent with other RPC methods that have a
'verbose' option.
Change the name of the return object from 'decoded' to details.
Update help text.
ACKs for top commit:
promag:
ACK 7dee8f48088c75ab0e51be60679505f8ce570919.
meshcollider:
Code review ACK 7dee8f48088c75ab0e51be60679505f8ce570919
0xB10C:
ACK 7dee8f48088c75ab0e51be60679505f8ce570919: reviewed code
Tree-SHA512: a3a62265c8e6e914591f3b3b9f9dd4f42240dc8dab9cbac6ed8d8b8319b6cc847db2ad1689d5440c162e0698f31e39fc6b868ed918b2f62879d61b9865cae66b
/**
* Dash specific comment
*
* Seems as event on_getdata works in our P2PInterface.
* But somehow it's never called for test 'test_in_flight_max',
* it may be due to bug in net_processing.
* Due to that, part of functional tests is disabled
*/
fab365835639a3da03f8ad9a58a0db6c6c4c2314 [qa] Test that getdata requests work as expected (Suhas Daftuar)
fa883ab35ad2d4328e35b1e855d0833740a6b910 net: Use mockable time for tx download (MarcoFalke)
Pull request description:
Two commits:
* First commit changes to mockable time for tx download (refactoring, should only have an effect on regtest)
* Second commit adds a test that uses mocktime to test tx download
ACKs for top commit:
laanwj:
code review ACK 16197/commits/fab365835639a3da03f8ad9a58a0db6c6c4c2314
jamesob:
ACK fab3658356
Tree-SHA512: 3a64a3e283ec4bab1f6e506404b11f0a564a5b61d2a7508ae738a61f035e57220484c66e0ae47d847fe9f7e3ff5cc834909d7b34a9bbcea6abe01f8742806908
Co-authored-by: UdjinM6 <UdjinM6@users.noreply.github.com>
fabca7756d6908ad581f3a699f1be6ecc9f62e03 doc: Add issue templates for bug and feature request (MarcoFalke)
Pull request description:
Fixes#16627
Can be tested via https://github.com/MarcoFalke/bitcoin/issues
ACKs for top commit:
jb55:
ACK fabca7756d6908ad581f3a699f1be6ecc9f62e03
fanquake:
ACK fabca7756d6908ad581f3a699f1be6ecc9f62e03
Tree-SHA512: 1ebe58f9c0110a9332adf1d80001cd9ed6fe60208e387c93b8564dc66821f753e34b23cb6f4cae45168024862ee884913976e132820b7a4759fa6391b0d1127c
9965940e35c445ccded55510348af228ff22f0e9 doc: Add release note for the new gettransaction argument (darosior)
b8b3f0435a2837d3897e9e232ef6ca839ce74eb8 tests: Add a new functional test for gettransaction (darosior)
7f3bb247a811582d1aa4805d8e601c19808dc7ba gettransaction: add an argument to decode the transaction (darosior)
Pull request description:
This PR adds a new parameter to the `gettransaction` call : `decode`. If set to `true`, it will add a new `decoded` field to the response. This mimics the behavior of `getrawtransaction`'s `verbose` argument to avoid using 2 calls if we want to decode a wallet transaction (`gettransaction` then `decoderawtransaction`).
Fix#16181 .
ACKs for top commit:
meshcollider:
re-utACK 9965940e35c445ccded55510348af228ff22f0e9
Tree-SHA512: bcb6b4bd252b3488d6afc77659c499c2ad99fd58661eb24b6a2e17014c74f22e47fde70e00fedb4f4754915786622ad02483b2cf2c4dea0ab0eb4ac8276dbeee
f3b57f4a1c17aadbf02d408e980490c88838c6ba Unrecommend making config file owned by bitcoin (setpill)
870d4152dfc3d990e336723562948835c2dbd646 Set ProtectHome in systemd service file (setpill)
639a416e3758b3005b860b198f0ec7bdd80a7f0c Chgrp config dir to bitcoin in systemd service (setpill)
aded0528f0e1e3735ce8dd26fd9e546150b73187 Improve clarity of systemd service file comments (setpill)
Pull request description:
Rationale: ran into a bug with the systemd service file, fixed it locally and figured I might as well contribute my fix.
Also fixed some unrelated confusing phrasing in the comments of the same file, after discussion in IRC.
ACKs for top commit:
sipsorcery:
tACK f3b57f4a1c17aadbf02d408e980490c88838c6ba (nothing changed since previous tACK).
ryanofsky:
utACK f3b57f4a1c17aadbf02d408e980490c88838c6ba. Only change since last review is removing ConfigurationDirectoryMode churn in early commits
Tree-SHA512: 2188345878925b9e8a5c2c3df8dfba443720e2252a164db54a8e1d8007846721497b2d98c56f1d9b60a9a9ed4fdb1156c7b02c699616b220a9b614671617d32a
593ba696fb32da558091ac02ad87c4893db4ce97 Add warning messages to the debug window (Hennadii Stepanov)
Pull request description:
Fix: #11016
This PR adds warning messages to the debug window in `-disablewallet` mode.
ACKs for top commit:
jonasschnelli:
utACK 593ba696fb32da558091ac02ad87c4893db4ce97
promag:
ACK 593ba696fb32da558091ac02ad87c4893db4ce97, agree with @Sjors https://github.com/bitcoin/bitcoin/pull/14879#pullrequestreview-196433092 above.
ryanofsky:
utACK 593ba696fb32da558091ac02ad87c4893db4ce97
Tree-SHA512: a8ca78529bb16813ba7bfaf5ccd4349189979f08e78ea857746a6fb00fd9d7ed98d8f06f384830acba21dac57070060af23f6be8249398feb32a6efff1333de8
14879 followup: move label_alerts css styling into css files and align it with the style of labelAlerts on OverviewPage
Co-authored-by: UdjinM6 <UdjinM6@users.noreply.github.com>
fa2056af1c travis: Properly cache and error on timeout (MarcoFalke)
fa36a333ee travis: Switch to ubuntu keyserver to avoid timeouts (MarcoFalke)
Pull request description:
The other keyserver is consistently timing out on travis: https://travis-ci.org/bitcoin/bitcoin/jobs/512689710#L405
Attempt to fix it by using a different server.
Also:
* fixes#15372
* fixes#15738
ACKs for commit fa2056:
ryanofsky:
utACK fa2056af1c71aded3a821a07ec4de71c4be0bca3. All good changes (changing keyserver, getting rid of keyserver while loop, clarifying travis error, moving travis documentation to code comment).
Tree-SHA512: ac8436616ecfee0ed579114e19f03c53ceb688fbcd95a60cffe8f15b4e569772a6ba673f353bbd789e79fe27fc5626c77fab4086768844dd51e0c6c108b52fb2
0fb2e69815 CreateTransaction: Assume minimum p2sh-p2wpkh spend size for unknown change (Gregory Sanders)
b06483c96a Remove stale comment in CalculateMaximumSignedInputSize (Gregory Sanders)
Pull request description:
This is triggered anytime a fundraw type call(psbt or legacy) is used with a change output address that the wallet doesn't know how to sign for.
This regression was added in 6a34ff5335 since BnB coin selection actually cares about this.
The fix is to assume the smallest typical spend, a P2SH-P2WPKH, which is calculated using a "prototype" dummy signature flow. Future work could generalize this infrastructure to get estimated sizes of inputs for a variety of types.
I also removed a comment which I believe is stale and misleading.
Tree-SHA512: c7e2be189e524f81a7aa4454ad9370cefba715e3781f1e462c8bab77e4d27540191419029e3ebda11e3744c0703271e479dcd560d05e4d470048d9633e34da16
5eb20f81d9 Consistently use ParseHashV to validate hash inputs in rpc (Ben Woosley)
Pull request description:
ParseHashV validates the length and encoding of the string and throws
an informative RPC error on failure, which is as good or better than
these alternative calls.
Note I switched ParseHashV to check string length first, because
IsHex tests that the length is even, and an error like:
"must be of length 64 (not 63, for X)" is much more informative than
"must be hexadecimal string (not X)" in that case.
Split from #13420
Tree-SHA512: f0786b41c0d7793ff76e4b2bb35547873070bbf7561d510029e8edb93f59176277efcd4d183b3185532ea69fc0bbbf3dbe9e19362e8017007ae9d51266cd78ae
