bb36372b8f test: add unit tests for Span-parsing helpers (Sebastian Falbesoner)
5e69aeec3f Add documenting comments to spanparsing.h (Pieter Wuille)
230d43fdbc Abstract out some of the descriptor Span-parsing helpers (Pieter Wuille)
Pull request description:
As suggested here: https://github.com/bitcoin/bitcoin/pull/16800#issuecomment-531605482.
This moves the Span parsing functions out of the descriptor module, making them more easily usable for other parsers (in particular, in preparation for miniscript parsing).
ACKs for top commit:
MarcoFalke:
ACK bb36372b8f
Tree-SHA512: b5c5c11a9bc3f0a1c2c4cfa22755654ecfb8d4b69da0dc1fb9f04e1556dc0f6ffd87ad153600963279ac465d587d7971b53d240ced802d12693682411ac73deb
a57a1d42d5 test: add unit test for wallet watch-only methods involving PubKeys (Sebastian Falbesoner)
Pull request description:
The motivation for this addition was to unit test the function `wallet.cpp:ExtractPubKey()` (see recent change in commit 798a589aff) which is however static and only indirectly available via the public methods `AddWatchOnly()`, `LoadWatchOnly()` and `RemoveWatchOnly()`. Since the first of those methods also stores the addresses to the disk, the second, simpler one was chosen which only operates in memory.
ACKs for top commit:
Sjors:
ACK a57a1d4
instagibbs:
reACK a57a1d42d5
Sjors:
re-ACK a57a1d4
Tree-SHA512: 92a242204ab533022cd848662997372c41815b1265d07b3d96305697f801db29a5ba5668337faf4bea702bec1451972529afd6665927fb142aaf91700a338b26
f4c8953b00 Add missing fields in TransactionDescriptionString and others (Antoine Riard)
3530108491 MOVEONLY : move RPC wallets helpers to TransactionDescriptionString (Antoine Riard)
Pull request description:
Knowledge of `walletconflicts` field existence is really nice when you're debugging conflicts. Was added in #3671 but never documented in RPC helps.
Others were added after a quick skim, we may still have missing ones in wallet rpcs.
ACKs for top commit:
MarcoFalke:
re-ACK f4c8953b00 (only change is addressing my nits)
kristapsk:
ACK f4c8953b00
Tree-SHA512: 2bea4d8743399fb152f942df7454548b896b2ad5654fd4bf60253afec1a5387ef8797ced97776dc7ba4912291263c08abe7c2b608c6a28f9a0df67be4ebc4635
5c2987636f tests: Remove TRANSACTION_DESERIALIZE (replaced by transaction fuzzer) (practicalswift)
0a573682f2 tests: Add fuzzing harness for CheckTransaction(...), IsStandardTx(...) and other CTransaction related functions (practicalswift)
Pull request description:
Add fuzzing harness for `CheckTransaction(...)`, `IsStandardTx(...)` and other `CTransaction` related functions.
**Testing this PR**
Run:
```
$ CC=clang CXX=clang++ ./configure --enable-fuzz --with-sanitizers=address,fuzzer,undefined
$ make
$ src/test/fuzz/transaction
…
# And to to quickly verify that the relevant code regions are triggered, that the
# fuzzing throughput seems reasonable, etc.
$ contrib/devtools/test_fuzzing_harnesses.sh '^transaction$'
```
`test_fuzzing_harnesses.sh` can be found in PR #17000.
ACKs for top commit:
MarcoFalke:
ACK 5c2987636f
Tree-SHA512: 2f422df795c9dca13c98209ca9ce0fe5a0d4a71fb052fa33d599cc9c9f1d637fee27d58d02ed17b956b3e3d40931cbc1367fc99aa2e882473e54d95dee04d6b7
facb9a1315 init: Change fallback locale to C.UTF-8 (Wladimir J. van der Laan)
Pull request description:
Much of our code assumes file system UTF-8 support, and this is a more realistic guess for modern systems anyway than the default character set (which would be ASCII only). So change the assumed fallback locale (if no locale is defined by the user or OS) to `C.UTF-8`.
related: https://github.com/bitcoin/bitcoin/issues/14948#issuecomment-488385462
ACKs for top commit:
MarcoFalke:
ACK facb9a1315
Tree-SHA512: 5075f9fe6791572d76ec38c58cd56f04ed8086c06a7d7f446d062dffc313c62466ba81f1a7d6b8c7e95791fcff82e4f76871c3534478fbfe5beb456dd8eea340
091747b46e gui: Add shortcuts for tab tools (João Barbosa)
Pull request description:
This makes accessing the RPC console very fast/easy. It also improves accessibility.
<img width="234" alt="Screenshot 2019-10-02 at 01 30 53" src="https://user-images.githubusercontent.com/3534524/66009867-50104300-e4b4-11e9-90b5-6b8dc961a8a1.png">
ACKs for top commit:
jonasschnelli:
Tested ACK 091747b46e - this is an improvment. Further solutions to solve the interference between the console and the shortcuts (if possible) can be done upstream (Qt) or with another PR.
Tree-SHA512: 6b8bc07e8a3a75e53c05f0fdb73458d75ef025f950569e885e655de53fdac8b91dcabfb1c6e643b1d23065420fa2701847c00cc1718bc188778640aefb5bcbd8
f33efa8ec5 GUI: Restore RPC Console to non-wallet tray icon menu (Luke Dashjr)
Pull request description:
#14383 moved the debug window's menu position, to make it conditional on wallet mode. The rationale given was to match the behaviour of the 'Help' menu.
#14573 replaced the 'Help' menu's conditional debug window with an unconditional list of items in the new 'Window' menu.
This PR reverts the no-longer-applicable part of #14383, putting the debug window back on the tray menu unconditionally, and in the position it previously had.
ACKs for top commit:
jonasschnelli:
Tested ACK f33efa8ec5 - the debug window is also accessible from the menu (though directly the subpages which counts IMO).
Tree-SHA512: c04a588fed37a8c31cb413baaa346e3c1c18724f9b40d64b8528c517f65290930d577bccf0a794180e968e84d3c52e9fa3fdc8a40bbc5fe3418eaddd73481271
181989f6c9 build: Add variable printing target to Makefiles (Carl Dong)
Pull request description:
```
I kept finding myself needing these to debug our build system, since
they are innocuous and are very helpful they probably belong in the
codebase.
Source: John Graham-Cumming
https://www.cmcrossroads.com/article/printing-value-makefile-variable
```
ACKs for top commit:
MarcoFalke:
ACK 181989f6c9
fanquake:
ACK 181989f6c9 - concise amount of useful code. Tested on macOS. Did not visit the link.
Tree-SHA512: 2139621e68a499c7347663ca9dc04e166ea6280e05986c27858df0156016ef2f9461262464d70c601419384f43a4ae3bcc67dfc0a05dbeef64f08386ab429cd8
cd82f75a43 lint: Install grep and git via brew on mac for --perl-regexp (Ben Woosley)
eafa747ca5 lint: Fix shellcheck SC2155 (Ben Woosley)
615ff4e7db lint: Run the linters against Mac OS on Travis (Ben Woosley)
Pull request description:
This helps ensure ongoing compatibility with macOS-distributed version of GNU bash.
ACKs for top commit:
MarcoFalke:
ACK cd82f75a43
Tree-SHA512: 8d56d2303bbebedba8ea2291f4ab35b7fdf3245b7a4c3f04557eee4f19d83573798ad32facc92bfa060aaeb294e6d2c95e6d1c3b795fd7951dcf3aa1cccec107
Fields involvesWatchonly, generated, walletconflicts were missing
in result description of listtransactions, listsinceblock,
gettransaction
Align getttransaction fields which were odd compare to other rpc
helpers
fa79dff624 ci: Run tests on arm (MarcoFalke)
Pull request description:
Closes#16576
ACKs for top commit:
laanwj:
ACK fa79dff624
fanquake:
ACK fa79dff624 - assuming Travis is green again.
Tree-SHA512: c430db9852632567c6836981fb3c5922ccd7d3b6ab4a1c6405ffad75096b80433ba54785ffa4c5088c1a127689a945f0f86058a42de1d3efea3cc4967832d662
a54ab2104c [doc] fix Makefile target in benchmarking.md (Sebastian Falbesoner)
Pull request description:
While the resulting binary is called `bench_bitcoin`, the Makefile target is
named `bitcoin_bench` (see `src/Makefile.bench.include`)
ACKs for top commit:
fanquake:
ACK a54ab2104c - Tested on macOS and Debian 9.9, as this only [seemed to work there](https://github.com/bitcoin/bitcoin/pull/16536#discussion_r310366868) when these docs were added.
Tree-SHA512: bcf8d48ccba488f0533111a3be57ddc6c948b3a38beed129635e1c7e0b4608bc9ddf625e8469606bb31d4cedf3341c443564a197d6b1ab5268a9ed44ed5018a3
9743432034 Fix bug where duplicate PSBT keys are accepted (John L. Jegutanis)
Pull request description:
As per the BIP 174 spec a PSBT key cannot be duplicated,
however the current code accepts key duplication.
The PSBT key/value entries can be duplicated when the value
is `empty()` or `IsNull()` for `CScript` or `CTxOut` respectively
and if those key/value entries are serialized before the non-empty ones.
For example, the following PSBT, included in the test vectors,
contains a duplicate field:
```
// magic
70736274ff
// global tx
//// key
0100
//// value
2a02000000000140420f000000000017a9146e91b72d5593e7d4391e2ff44e91e985c31641f08700000000
//// separator
00
// no inputs
// outputs
//// key PSBT_OUT_WITNESSSCRIPT
0101
//// value (empty script)
00
//// key PSBT_OUT_WITNESSSCRIPT (same as the above)
0101
//// value (an OP_RETURN script)
016a
//// separator
00
```
ACKs for top commit:
achow101:
ACK 9743432034
instagibbs:
code review ACK 9743432034
Tree-SHA512: 34f4b34c8e6561c6a6ab745cdd319f6687eac6f7cecc735c94035eeca8c5157e17a27f2ae853dbaa6634fcd5a8f4e1c6cc13d1ebd7e563459665d72bb147cc1e
Much of our code assumes UTF-8 support, and this is a more realistic
guess for modern systems anyway than the default character set (which
would be ASCII only). So change the assumed fallback locale (if no
locale is defined by the user or OS) to `C.UTF-8`.
fa25f43ac5 p2p: Remove BIP61 reject messages (MarcoFalke)
Pull request description:
Reject messages (BIP 61) appear in the following settings:
* Parsing of reject messages (in case `-debug=net` is set, off by default). This has only been used for a single `LogPrint` call for several releases now. Such logging is completely meaningless to us and should thus be removed.
* The sending of reject messages (in case `-enablebip61` is set, off by default). This can be used to debug a node that is under our control. Instead of hacking this debugging into the p2p protocol, it could be more easily achieved by parsing the debug log. (Use `-printtoconsole` to have it as stream, or read from the `debug.log` file like our python function `assert_debug_log` in the test framework does)
Having to maintain all of this logic and code to accommodate debugging, which can be achieved by other means a lot easier, is a burden. It makes review on net processing changes a lot harder, since the reject message logic has to be carried around without introducing any errors or DOS vectors.
ACKs for top commit:
jnewbery:
utACK fa25f43ac5
laanwj:
I'm still not 100% convinced that I like getting rid of BIP61 conceptually, but apparently everyone wants it, code review ACK fa25f43ac5.
ryanofsky:
Code review ACK fa25f43ac5
Tree-SHA512: daf55254202925e56be3d6cfb3c1c804e7a82cecb1dd1e5bd7b472bae989fd68ac4f21ec53fc46751353056fd645f7f877bebcb0b40920257991423a3d99e0be
9d1f971c75 gitian: Put things in the right place to begin with (Carl Dong)
71949a97a7 gitian: Eliminate rename dependency (Carl Dong)
999a9a5f5b gitian: Smaller diff with gitian-linux.yml (Carl Dong)
c4a3c25ba1 gitian: Fix README inclusion in archives (Carl Dong)
93cb974980 gitian: Use split-debug.sh for Win builds (Carl Dong)
Pull request description:
It would seem that our `gitian-win.yml` has not been keeping up with `gitian-linux.yml`, this PR:
1. Minimizes the diff size between `gitian-{win,linux}.yml`
2. Eliminates the `rename` dependency
ACKs for top commit:
laanwj:
ACK 9d1f971c75
Tree-SHA512: 84ed47c685e12d0064c02811907ae3d0fd3c47db8773d497dcc38f0defbfb3040fd82899fb026cf355f229b906d05a1c8038a95642bb90d044afbc2e0b239af2
4bb660be90 Add release note (Andrew Chow)
ed96b295d7 Update descriptors.md to include sortedmulti (Andrew Chow)
80be78ea75 Test sortedmulti descriptor using BIP 67 tests (Andrew Chow)
6f588fd227 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 4bb660be90
Tree-SHA512: 93b21112a74ebe0bf316d8f3e0291f69fd975cf0a29332f9728e7b880cad312b8b14007e86adcd7899f117b9303cbcf4cb35f3bb2f2f648d1a446f83f75a70a5
Linux:
The README was originally added in 8550f1fb2, but included the README
under the docs directory, which has a bunch of internal links that won't
make sense in a release tarball. In this patch, we include the root
level README instead, which makes more sense.
Windows:
.md files are inconvenient to open on windows and the line endings
differ, so we use README_windows.txt instead.
c640ca24f9 Always generate `bitcoinstrings.cpp` on `make translate` (Franck Royer)
Pull request description:
Resolves#16891.
`bitcoinstrings.cpp` is to be generated at release time. Hence,
it should not depend on whether the source files are younger as the
releaser may proceed from a fresh checkout.
More information on the investigation in the issue.
ACKs for top commit:
jonasschnelli:
Tested ACK c640ca24f9 - Tested master + this on macOS 10.14. With this PR, it always "runs" `GEN qt/bitcoinstrings.cpp` and `GEN translate`.
Tree-SHA512: f799fdc3ad16a2a6a59704bc2c50f5179e6a7e064d8f43354592f11857cc901cac99b2b90f3319d25d49c9d78378b8d119cc5f59b48ea7f1008f33dd26700877
30fc1a3f54 build: Remove workaround for ancient libtool (Hennadii Stepanov)
6ca01b9a10 build: Ensure a minimal version of libtool (Hennadii Stepanov)
Pull request description:
Since libtool 1.5.2, on Linux libtool no longer sets RPATH for any directories in the dynamic linker search path, so there is no longer an issue.
This commit reverts a98356fee8.
Refs:
- https://wiki.debian.org/RpathIssue
- [Debian jessie has libtool 2.4.2](https://packages.debian.org/jessie/libtool)
ACKs for top commit:
laanwj:
ACK 30fc1a3f54
Tree-SHA512: fab56265d4d2c96216a353cc076c6f510e15748d8134f97bae2f67b6d8c0b6a1a9f362d2ab23b19ccc3a8bba8eac3bb1668fc3e42037590f63a7ab4819c9ee15
7fb7acfc20 Set init stop timeout to 10 min (setpill)
Pull request description:
`bitcoind` can take a long time to flush its db cache to disk upon
shutdown. Systemd sends a `SIGKILL` after a timeout, causing unclean
shutdowns and triggering a long "Rolling forward" at the next startup.
Disabling the timeout should prevent this from happening, and does not
break systemd's `restart` logic.
Addresses #13736.
ACKs for top commit:
instagibbs:
utACK 7fb7acfc20
Tree-SHA512: 16e0ce5a9ecf0628f8d93d68db3f5a78ab36021d9bede05a90c84f144db2e87e17707a6eb910cb7c018c265ce2c81d43de2988bd79e4a2d8554515db8fb5aa36
ddddd8961b ci: Use busybox utils for one build (MarcoFalke)
Pull request description:
To make sure Bitcoin Core can be built with BusyBox, see https://github.com/bitcoin/bitcoin/pull/16927#issuecomment-536483706
ACKs for top commit:
laanwj:
ACK ddddd8961b
Tree-SHA512: da3a4654ee7975206d04643675d309b4973a510ca344acaec97fb1ed19c43cf13489bdf236c92c4a90499ec5b3c18c3338fff096110b26abee5ffe955089f267
0d86f4d3da refactor: consolidate PASTE macros (James O'Beirne)
Pull request description:
Really dumb move-only stolen from #16805. Some of my pull requests also depend on this, so I split it up to not depend on #16805.
ACKs for top commit:
practicalswift:
ACK 0d86f4d3da -- diff looks correct
hebasto:
ACK 0d86f4d3da, I have reviewed the code and it looks OK, I agree it can be merged.
promag:
ACK 0d86f4d3da.
Tree-SHA512: 19208a8cbf83034b1ef25138d8f08d8f32ace7775f654b1597fc4599dd576f0758145f592f161cfdcaaa29d4907ac9aa5553f6f524e2b960205c760605a05901
3284e6c09a scripts: search for next position of magic bytes rather than fail (Tim Akinbo)
Pull request description:
When using the `linearize-data.py` contrib script to export block data, there are edge cases where the script fails with an `Invalid magic: 00000000` error. This error occurs due to the presence of padding bytes that occasionally appears between consecutive blocks in the block data file.
There's an ongoing conversation about this in #14986. sipa also admitted that it is a bug in #5028. Fortunately, this is not an issue in bitcoin core as it handles this type of situation gracefully and so no fix in bitcoin core is required.
This PR is an improvement on how the script handles these "invalid magic bytes". Rather than failing, this patch allows the script to search for the next occurrence of the magic bytes and then starts reading the block from there.
ACKs for top commit:
laanwj:
ACK 3284e6c09a
Tree-SHA512: 18067ae0b4b62e822dfc558a86439ad6acaf939b98479e38e8e4248536574643b26eb48e96ec7139375c88b42cbe7705a64deb13a3c239e16025a6aad3d69bfa
8acd58927a Fix Python Docstring to include all Args. (John Bampton)
Pull request description:
Found a Python function that had incorrect and missing arguments in its Docstring.
ACKs for top commit:
laanwj:
ACK 8acd58927a
Tree-SHA512: 936f275f29a700d630bb479b5283e47b66f2df76d8b8c053f594e6aedf783cc98a29c924c3a46613f112dfc884acb50f21a0b18f96d939e887b12b921ef2e10f
As per the BIP 174 spec a PSBT key cannot be duplicated,
however the current code accepts key duplication.
The PSBT key/value entries can be duplicated when the value
is `empty()` or `IsNull()` for `CScript` or `CTxOut` respectively
and if those key/value entries are serialized before the non-empty ones.
For example, the following PSBT, included in the test vectors,
contains a duplicate field:
```
// magic
70736274ff
// global tx
//// key
0100
//// value
2a02000000000140420f000000000017a9146e91b72d5593e7d4391e2ff44e91e985c31641f08700000000
//// separator
00
// no inputs
// outputs
//// key PSBT_OUT_WITNESSSCRIPT
0101
//// value (empty script)
00
//// key PSBT_OUT_WITNESSSCRIPT (same as the above)
0101
//// value (an OP_RETURN script)
016a
//// separator
00
```
ffa2221256 tests: Pass fuzzing inputs as constant references (practicalswift)
Pull request description:
Pass fuzzing inputs as constant references.
Split out from #17009 as suggested by MarcoFalke in https://github.com/bitcoin/bitcoin/pull/17009#discussion_r331502028.
ACKs for top commit:
MarcoFalke:
ACK ffa2221256
Tree-SHA512: c1e3d6658a0b45cece2ed7e7c2ba1b78cdb71a03767231de7e5c212575117a4e2b70079265c745963480d7fcd4d9706561a2325b8c993b94eec33268ad4b293c
