Commit graph

22964 commits

Author SHA1 Message Date
MarcoFalke
fab30b61eb
util: Remove unused ParseMoney that takes a c_str 2020-02-29 00:25:37 +07:00
MarcoFalke
e5753fa4e8
Merge #17921: test: test OP_CSV empty stack fail in feature_csv_activation.py
5ffaf883b9 test: eliminiated magic numbers in feature_csv_activation.py (Sebastian Falbesoner)
09f706ab8e test: check for OP_CSV empty stack fail reject reason in feature_csv_activation.py (Sebastian Falbesoner)
cbd345a75c test: test OP_CSV empty stack fail in feature_csv_activation.py (Sebastian Falbesoner)

Pull request description:

  Adds an empty stack failure check for OP_CSV (BIP112) to the functional test `feature_csv_activation.py` by prepending a valid scriptSig with `OP_CHECKSEQUENCEVERIFY`.
  If BIP112 is inactive, the operator just behaves as a NOP (for both tx versions 1 and 2) and the transaction remains valid -- if it is active, the tx is invalid due to an empty stack (for both tx versions 1 and 2, as well).

Top commit has no ACKs.

Tree-SHA512: 81102aaead5be11e02b894867fa9a9cc17358ec0eb2f21ce2d3db845b87691d305e6ed7c525f9c7e5bcb3c5c609eb28deca0fbaa3d5e9ff928cecd3b91ff129a
2020-02-28 22:49:29 +07:00
Sebastian Falbesoner
5ffaf883b9 test: eliminiated magic numbers in feature_csv_activation.py 2020-02-28 09:04:59 +01:00
Sebastian Falbesoner
09f706ab8e test: check for OP_CSV empty stack fail reject reason in feature_csv_activation.py 2020-02-28 09:04:59 +01:00
Sebastian Falbesoner
cbd345a75c test: test OP_CSV empty stack fail in feature_csv_activation.py
With BIP112 activated, the operation OP_CHECKSEQUENCEVERIFY (former OP_NOP3)
leads to script interpreter termination with an error if one of the following
conditions is true:
    -> stack is empty
    -> top item on stack is negative (< 0)
    -> top item on stack has disable flag unset and at least one of
       four other conditions is true (contains the core CSV logic)

This commits adds the missing empty stack failure test to the functional test
by prepending a valid scriptSig with just OP_CHECKSEQUENCEVERIFY. If BIP112 is
inactive, the operator just behaves as a NOP (for both tx versions 1 and 2) and
the transaction remains valid -- if it is active, the tx is invalid due to an
empty stack (for both tx versions 1 and 2, as well).
2020-02-28 09:04:18 +01:00
fanquake
eae48ec84c
Merge #18209: test: Reduce unneeded whitelist permissions in tests
fa45d60646 test: Reduce unneeded whitelist permissions in tests (MarcoFalke)

Pull request description:

  It makes the tests confusing and fragile when overwriting default command line values that are not needed to be overwritten.

ACKs for top commit:
  fanquake:
    ACK fa45d60646
  laanwj:
    ACK fa45d60646

Tree-SHA512: 8ae5ad8c6be156b1a983adccbca8d868ef841e00605ea88e24227f1b7493987c50b3e62e68dd7dc785ad73d6e14279eb13d7a151cb0a976426fe2fd63ce5cbcd
2020-02-28 12:32:31 +08:00
fanquake
fe63d79eab
Merge #18212: doc: add missing step in win deployment instructions
7644567758 Add missing step in win deployment instructions (Dan Gershony)

Pull request description:

  As explained in #17864 there is a missing step that was required to finish the compilation for Bitcoin Core on Windows.

ACKs for top commit:
  sipsorcery:
    ACK 7644567758.

Tree-SHA512: 0d9ed248f511ea4f440d6c2f3e1235abbb3f9c0c576ca715df3cda91682d668991001197930e687ee48709eedbcf148d8ac9236464e9ce1d2ed15d8b3b4b252d
2020-02-28 08:37:03 +08:00
MarcoFalke
1615043935
Merge #17461: test: check custom descendant limit in mempool_packages.py
b902bd66b0 test: check custom descendant limit in mempool_packages.py (Sebastian Falbesoner)

Pull request description:

  This is a follow-up PR to #17435, testing the custom descendant limit, passed by the argument `-limitdescendantcount`. ~~It was more tricky than expected, mainly because we don't know for sure at which point node1 has got all the transactions broadcasted from node0 (for the ancestor test this wasn't a problem since the txs were immediately available through `invalidateblock`) -- a simple `sync_mempools()` doesn't work here since the mempool contents are not equal due to different ancestor/descendant limits. Hence I came up with a "hacky manual sync":~~
  1. ~~wait until the mempool has the _expected_ tx count (see conditions below)~~
  2. ~~after that, wait some time and get sure that the mempool contents haven't changed in-between~~

  ~~Like for~~ Similar to the ancestor test, we overall check for ~~three~~ four conditions:
  - the # of txs in the node1 mempool is equal to the descendant limit (plus 1 for the parent tx, plus the # txs from the previous ancestor test which are still in) ~~(done by the hacky sync above)~~
  - all txs in node1 mempool are a subset of txs in node0 mempool
  - part of the constructed descendant-chain (the first ones up to the limit) are contained in node1 mempool
  - the remaining part of the constructed descendant-chain (all after the first ones up to the limit) is *not* contained in node1 mempool

ACKs for top commit:
  JeremyRubin:
    Excellent. utACK b902bd6

Tree-SHA512: 7de96dd248f16ab740e178ac5b64b57ead18cdcf74adfe989709d215e4a67b6b6d20de22c48e885d5f2edc55caaddd44a4261e996c5c87687ceb6a47f1d1fdaf
2020-02-28 03:02:24 +07:00
MarcoFalke
324a6dfeaf
Merge #17771: tests: Add fuzzing harness for V1TransportDeserializer (P2P transport)
2f63ffd15c tests: Add fuzzing harness for V1TransportDeserializer (P2P transport) (practicalswift)

Pull request description:

  Add fuzzing harness for `V1TransportDeserializer` (P2P transport).

  **Testing this PR**

  Run:

  ```
  $ make distclean
  $ ./autogen.sh
  $ CC=clang CXX=clang++ ./configure --enable-fuzz \
        --with-sanitizers=address,fuzzer,undefined
  $ make
  $ src/test/fuzz/p2p_transport_deserializer
  …
  ```

ACKs for top commit:
  MarcoFalke:
    ACK 2f63ffd15c

Tree-SHA512: 8507d4a0414d16f1b8cc9649e3e638f74071dddc990d7e5d7e6faf77697f50bdaf133e49e2371edd29068a069a074469ef53148c6bfc9950510460b81d87646a
2020-02-28 02:35:14 +07:00
Dan Gershony
7644567758
Add missing step in win deployment instructions
As explained in #17864  there is a missing step that was required to finish the compilation for bitcoin core on windows
2020-02-27 15:11:30 +01:00
fanquake
4502ed7cd1
Merge #18211: test: Disable mockforward scheduler unit test for now
fab2527515 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 fab2527515 - be good to get Travis back.
  laanwj:
    ACK fab2527515

Tree-SHA512: 027e86b3dfec203a464e5bf528e9933c208c36633c2d4bfcdbc10da1799637a5d6ea0a63af33a4174fb1ad7115df631a4cb838f56e31f4cbd15498e1e9fdf9cc
2020-02-27 09:42:13 +08:00
Sebastian Falbesoner
b902bd66b0 test: check custom descendant limit in mempool_packages.py
To test the custom descendant limit on node1 (passed by the argument
-limitdescendantcount), we check for four conditions:
    -> the # of txs in the node1 mempool is equal to the limit
       (plus 1 for the parent tx, plus the # txs from the previous ancestor
        test which are still in)
    -> all txs in node1 mempool are a subset of txs in node0 mempool
    -> part of the constructed descendant-chain (the first ones up to the
       limit) are contained in node1 mempool
    -> the remaining part of the constructed descendant-chain (all after the
       first ones up to the limit) is *not* contained in node1 mempool
2020-02-27 00:24:33 +01:00
Wladimir J. van der Laan
e5d47ed8fd
Merge #18167: Fix a violation of C++ standard rules where unions are used for type-punning
0653939ac1 Add static_asserts to ser_X_to_Y() methods (Samer Afach)
be94096dfb 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 0653939ac1
  elichai:
    ACK 0653939ac1
  laanwj:
    Code review ACK 0653939ac1
  kristapsk:
    ACK 0653939ac1

Tree-SHA512: f6e89de39fc964750429139bab6b5a1346f7060334b7afa020e315bdad8f8c195bce2b8a9e343f06e7fff175e2dfb1cdabfcb6fe405bea0febe4962f0cc62557
2020-02-26 19:00:24 +01:00
Wladimir J. van der Laan
89a97a71f2
Merge #17985: net: Remove forcerelay of rejected txs
facb71576c 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 4d8993b346, 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 facb71576c, locally running the unit and functional tests.

Tree-SHA512: bfceae6f2983c1510fa0649a9a63c343cbbc1c4ab3a3698039cccf454c81e58c8f5114b147ed42a1bc867da74c43a5b53764ab14f942e191b6f59079044108b5
2020-02-26 18:46:05 +01:00
MarcoFalke
fab2527515
test: Disable mockforward scheduler unit test for now 2020-02-27 00:19:16 +07:00
MarcoFalke
c3b4715923
Merge #18206: tests: Add fuzzing harness for bloom filter classes (CBloomFilter + CRollingBloomFilter)
eabbbe409f tests: Add fuzzing harness for rolling bloom filter class CRollingBloomFilter (practicalswift)
2a6a6ea0f5 tests: Add fuzzing harness for bloom filter class CBloomFilter (practicalswift)

Pull request description:

  Add fuzzing harness for bloom filter classes (`CBloomFilter` + `CRollingBloomFilter`).

  Test this PR using:

  ```
  $ make distclean
  $ ./autogen.sh
  $ CC=clang CXX=clang++ ./configure --enable-fuzz \
        --with-sanitizers=address,fuzzer,undefined
  $ make
  $ src/test/fuzz/bloom_filter
  …
  $ src/test/fuzz/rolling_bloom_filter
  …
  ```

ACKs for top commit:
  MarcoFalke:
    ACK eabbbe409f 🤞

Tree-SHA512: 765d30bc52e3eb04dbd4d2b8f517387aa61312416e8fea3767250ef5c074e08641699019ee4600d42303de32f98379c20bfc0c0e60cb5154d0338088c1d29cb6
2020-02-26 02:37:43 +07:00
practicalswift
eabbbe409f tests: Add fuzzing harness for rolling bloom filter class CRollingBloomFilter 2020-02-25 17:04:03 +00:00
practicalswift
2a6a6ea0f5 tests: Add fuzzing harness for bloom filter class CBloomFilter 2020-02-25 17:04:03 +00:00
MarcoFalke
fa45d60646
test: Reduce unneeded whitelist permissions in tests 2020-02-26 00:00:29 +07:00
Samuel Dobson
31c0006a6c
Merge #17264: rpc: set default bip32derivs to true for psbt methods
5bad7921d0 [test] PSBT RPC: check that bip32_derivs are present by default (Sjors Provoost)
29a21c9061 [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 5bad7921d0

Tree-SHA512: 22ad71dda96856060a96758c4ae7aafa22d5e9efba30e0c8287c711e7579849bd72593cbc0f41a2e9e8821315d78bda04e848dbb006283b841b2795e2faebcfd
2020-02-25 23:50:39 +13:00
Samuel Dobson
03f98b15ad
Merge #17577: refactor: deduplicate the message sign/verify code
e193a84fb2 Refactor message hashing into a utility function (Jeffrey Czyz)
f8f0d9893d Deduplicate the message signing code (Vasil Dimov)
2ce3447eb1 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 e193a84fb2
  achow101:
    ACK e193a84fb2
  instagibbs:
    utACK e193a84fb2
  meshcollider:
    utACK e193a84fb2

Tree-SHA512: b0e02a7d4623a98c8f8c77627af1725e6df07700de4630c2f75da6beacdf55414c38ba147bc6d2a757491ab07c827dddf93e8632fe600478760e255714ddab88
2020-02-25 23:29:54 +13:00
fanquake
a674e89d27
Merge #18162: util: Avoid potential uninitialized read in FormatISO8601DateTime(int64_t) by checking gmtime_s/gmtime_r return value
12a2f37718 util: Avoid potential uninitialized read in FormatISO8601DateTime(int64_t nTime) by checking gmtime_s/gmtime_r return value (practicalswift)

Pull request description:

  Avoid potential uninitialized read in `FormatISO8601DateTime(int64_t)` by checking `gmtime_s`/`gmtime_r` return value.

  Before this patch `FormatISO8601DateTime(67768036191676800)` resulted in:

  ```
  ==5930== Conditional jump or move depends on uninitialised value(s)
  ==5930==    at 0x4F44C0A: std::ostreambuf_iterator<char, std::char_traits<char> > std::num_put<char, std::ostreambuf_iterator<char, std::char_traits<char> > >::_M_insert_int<long>(std::ostreambuf_iterator<char, std::char_traits<char> >, std::ios_base&, char, long) const (in /usr/lib/x86_64-linux-gnu/libstdc++.so.6.0.25)
  ==5930==    by 0x4F511A4: std::ostream& std::ostream::_M_insert<long>(long) (in /usr/lib/x86_64-linux-gnu/libstdc++.so.6.0.25)
  ==5930==    by 0x4037C3: void tinyformat::formatValue<int>(std::ostream&, char const*, char const*, int, int const&) (tinyformat.h:358)
  ==5930==    by 0x403725: void tinyformat::detail::FormatArg::formatImpl<int>(std::ostream&, char const*, char const*, int, void const*) (tinyformat.h:543)
  ==5930==    by 0x402E02: tinyformat::detail::FormatArg::format(std::ostream&, char const*, char const*, int) const (tinyformat.h:528)
  ==5930==    by 0x401B16: tinyformat::detail::formatImpl(std::ostream&, char const*, tinyformat::detail::FormatArg const*, int) (tinyformat.h:907)
  ==5930==    by 0x4017AE: tinyformat::vformat(std::ostream&, char const*, tinyformat::FormatList const&) (tinyformat.h:1054)
  ==5930==    by 0x401765: void tinyformat::format<int, int, int, int, int, int>(std::ostream&, char const*, int const&, int const&, int const&, int const&, int const&, int const&) (tinyformat.h:1064)
  ==5930==    by 0x401656: std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > tinyformat::format<int, int, int, int, int, int>(char const*, int const&, int const&, int const&, int const&, int const&, int const&) (tinyformat.h:1073)
  ==5930==    by 0x4014CC: FormatISO8601DateTime[abi:cxx11](long) (…)
  ```

  The same goes for other very large positive and negative arguments.

  Fix by simply checking the `gmtime_s`/`gmtime_r` return value :)

ACKs for top commit:
  MarcoFalke:
    ACK 12a2f37718
  theStack:
    re-ACK 12a2f37718
  elichai:
    re ACK 12a2f37718

Tree-SHA512: 066142670d9bf0944d41fa3f3c702b1a460b5471b93e76a619b1e818ff9bb9c09fe14c4c37e9536a04c99533f7f21d1b08ac141e1b829ff87ee54c80d0e61d48
2020-02-25 10:06:38 +08:00
MarcoFalke
225aa5d6d5
Merge #18193: scripted-diff: Wallet: Rename incorrectly named *UsedDestination
bca8665d08 scripted-diff: Wallet: Rename incorrectly named *UsedDestination (Luke Dashjr)

Pull request description:

  These functions are used to mark/check if a key of our own has been used to spend (and only for avoid-reuse wallets), which has nothing to do with the destination/address itself.
  Give them more accurate names to avoid confusion.

  -BEGIN VERIFY SCRIPT-
  sed -i -e 's/UsedDestination/SpentKey/g' $(git grep -l 'UsedDestination' ./src)
  -END VERIFY SCRIPT-

ACKs for top commit:
  practicalswift:
    ACK bca8665d08 -- patch looks correct and rationale makes sense
  instagibbs:
    ACK bca8665d08, much more meaningful name, thanks
  kallewoof:
    ACK bca8665d08

Tree-SHA512: ff13d9061ffa748e92eb41ba962c3ec262a43e4b6abd62408b38c6f650395d6ae5851554257d1900fb02767a88d08380d592a27210192ee9abb72d0945976686
2020-02-24 23:01:24 +07:00
MarcoFalke
ab9de43588
Merge #18181: test: Remove incorrect assumptions in validation_flush_tests
faca8eff39 test: Remove incorrect assumptions in validation_flush_tests (MarcoFalke)
fa31eebfe9 test: Tabs to spaces in all tests (MarcoFalke)

Pull request description:

  The tests assume standard library internals that may not hold on all supported archs or when the code is instrumented for sanitizer or debug use cases

  Fixes #18111

ACKs for top commit:
  jamesob:
    ACK faca8eff39 pending passing tests
  fjahr:
    ACK faca8eff39

Tree-SHA512: 60a5ae824bdffb0762f82f67957b31b185385900be5e676fcb12c23d53f5eea734601680c2e3f0bdb8052ce90e7ca1911b1342affb67e43d91a506b111406f41
2020-02-22 22:18:46 +07:00
MarcoFalke
36e507227e
Merge #18183: test: Set catch_system_errors=no on boost unit tests
fac52dafa0 test: Set catch_system_errors=no on boost unit tests (MarcoFalke)

Pull request description:

  Closes #16700

  Can be tested by adding an `assert(0)` and then running either `make check` or `./src/test/test_bitcoin -t bla_tests --catch_system_errors=no/yes`

ACKs for top commit:
  practicalswift:
    ACK fac52dafa0
  Empact:
    Tested ACK fac52dafa0

Tree-SHA512: ec00636951b2c1137aaf43610739d78d16f823f7da76a726d47f93b8b089766fb66b21504b3c5413bcf8b6b5c3db0ad74027d677db24a44487d6d79a6bdee2e0
2020-02-21 15:00:22 -08:00
Luke Dashjr
bca8665d08 scripted-diff: Wallet: Rename incorrectly named *UsedDestination
These functions are used to mark/check if a key of our own has been used to spend (and only for avoid-reuse wallets), which has nothing to do with the destination/address itself.
Give them more accurate names to avoid confusion.

-BEGIN VERIFY SCRIPT-
sed -i -e 's/UsedDestination/SpentKey/g' $(git grep -l 'UsedDestination' ./src)
-END VERIFY SCRIPT-
2020-02-21 21:16:40 +00:00
Samuel Dobson
9dd7bd47be
Merge #18034: Get the OutputType for a descriptor
7e80f646b2 Get the OutputType for a descriptor (Andrew Chow)

Pull request description:

  Adds a `GetOutputType()` method to get the OutputType of a descriptor. Some descriptors don't have a determinate OutputType, so we actually use an `Optional<OutputType>`. For descriptors with indeterminate OutputType, we return `nullopt`.

  `addr()` and `raw()` use OutputTypes as determined by the CTxDestination they have. For simplicity, `ScriptHash` destinations are `LEGACY` even though they could be `P2SH_SEGWIT`.
  `combo()`, `pk()`, and `multi()` are `nullopt` as they either don't have an OutputType or they have multiple. `DescriptorImpl` defaults to `nullopt`.
  `pkh()` is `LEGACY` as expected
  `wpkh()` and `wsh()` are `BECH32` as expected.
  `sh()` checks whether the sub-descriptor is `BECH32`. If so, it is `P2SH_SEGWIT`. Otherwise it is `LEGACY`.

  The descriptor tests are updated to check the OutputType too.

ACKs for top commit:
  fjahr:
    ACK 7e80f646b2
  meshcollider:
    utACK 7e80f646b2
  instagibbs:
    cursory ACK 7e80f646b2
  Sjors:
    Code review ACK 7e80f646b2
  jonatack:
    ACK 7e80f64 code review/build/tests

Tree-SHA512: c5a813447b62e982435e1c948066f8d6c148c9ebffb0a5eb5a9028b173b01d5ead2f076a5ca3f7f37698538baa346f82a977ee48f583d89cb4e5ebd9111b2341
2020-02-22 08:02:52 +13:00
MarcoFalke
e9fc8f6e7f
Merge #18172: test: Transaction expiry from mempool
d6d2602a32 add: test that transactions expire from mempool (0xb10c)

Pull request description:

  This adds the functional test `mempool_expiry.py` covering mempool transaction expiry. Both the default `DEFAULT_MEMPOOL_EXPIRY` of 336 hours (two weeks, set in #9312) and the user definable mempool expiry via the `-mempoolexpiry=<n>` command line option are tested. The test checks that descendants of expired transactions are removed as well.

  *Notes for reviewers*
  - `LimitMempoolSize()` (which is the only caller of `CTxMemPool::Expire()`) is only called when a transaction is added to the mempool. In order to test expiry of a transaction-that-should-expire, the mocktime is set and a random transaction is broadcast to trigger `LimitMempoolSize()`. The transaction-that-should-expire is then checked for expiry. LMK if there is another way, but I don't think there is.

ACKs for top commit:
  MarcoFalke:
    ACK d6d2602a32
  theStack:
    ACK d6d2602a32
  promag:
    Code review ACK d6d2602a32.

Tree-SHA512: eb68cd9e2d870872b8e8e1522fed8954fb99cc9e4edda4b28bb2a4e41cddbc53fe6f7d9c090f1e0e98ab49beb24bf37ff3787a9e9801a95e8ae9ca9eb34fe6f0
2020-02-21 10:23:20 -08:00
fanquake
eb3c6b0912
Merge #18070: doc: add note about brew doctor
63ce882760 doc: link to homebrew's troubleshooting page (Gastón I. Silva)

Pull request description:

  A trivial documentation update.

  When I was following the build steps for mac, I had some errors installing the dependencies. After searching on the Internet, and correcting the errors, I found that `brew doctor` had all the answers I needed. Could have skipped the Internet searches all together.

ACKs for top commit:
  fanquake:
    ACK 63ce882760 - a link to the troubleshooting page seems fine. I wouldn't really want our README to have anything more specific than that.

Tree-SHA512: 12c96cd9c9bd39ada21f3f27cbec3ed4bef4b8e74dec7872c892fc6a92a70418a5cc0882ff449883e91d96c01e1ca7104b076590917f397334c82931ec7fda1c
2020-02-20 20:56:41 +08:00
fanquake
56fc2dfcc3
Merge #18122: rpc: update validateaddress RPCExamples to bech32
7f1475c711 rpc: update validateaddress RPCExamples to bech32 (Sebastian Falbesoner)

Pull request description:

  Another small step to get rid of legacy addresses in the RPC help texts and by that encourage the use of bech32 addresses by default. The (invalid) address is the same as in the `getaddressinfo` RPC (see 2ee0cb3330, kudos to jonatack!), I don't think it adds any value to have a different example address per RPC.

ACKs for top commit:
  fanquake:
    ACK 7f1475c711
  MarcoFalke:
    ACK 7f1475c711

Tree-SHA512: 2350f61fa942a9053f9f5c860ea446965dc7209c71c81bdb98a859d03ca23b225ad72c9c506e4a55c8d8988823d9cfbe808c1a452a1eeadb70ab186b146dd4ca
2020-02-20 20:28:46 +08:00
MarcoFalke
fac52dafa0
test: Set catch_system_errors=no on boost unit tests 2020-02-19 16:14:50 -08:00
practicalswift
12a2f37718 util: Avoid potential uninitialized read in FormatISO8601DateTime(int64_t nTime) by checking gmtime_s/gmtime_r return value 2020-02-19 22:41:06 +00:00
MarcoFalke
faca8eff39
test: Remove incorrect assumptions in validation_flush_tests 2020-02-19 11:52:25 -08:00
MarcoFalke
fa31eebfe9
test: Tabs to spaces in all tests
Spaces are used in all of the source code except in these two instances
2020-02-19 11:51:40 -08:00
Samer Afach
0653939ac1 Add static_asserts to ser_X_to_Y() methods 2020-02-19 18:44:46 +01:00
MarcoFalke
eddcbfb109
Merge #18166: ci: Run fuzz testing test cases (bitcoin-core/qa-assets) under valgrind to catch memory errors
f2472f6460 tests: Improve test runner output in case of target errors (practicalswift)
733bbec34f tests: Add --exclude integer,parse_iso8601 (temporarily) to make Travis pass until uninitialized read issue in FormatISO8601DateTime is fixed (practicalswift)
5ea81449f3 tests: Add support for excluding fuzz targets using -x/--exclude (practicalswift)
555236f769 tests: Remove -detect_leaks=0 from test/fuzz/test_runner.py - no longer needed (practicalswift)
a3b539a924 ci: Run fuzz testing test cases under valgrind (practicalswift)

Pull request description:

  Run fuzz testing [test cases (bitcoin-core/qa-assets)](https://github.com/bitcoin-core/qa-assets) under `valgrind`.

  This would have caught `util: Avoid potential uninitialized read in FormatISO8601DateTime(int64_t) by checking gmtime_s/gmtime_r return value` (#18162) and similar cases.

ACKs for top commit:
  MarcoFalke:
    ACK f2472f6460 👼

Tree-SHA512: bb0879d40167cf6906bc0ed31bed39db83c39c7beb46026f7b0ee53f28ff0526ad6fabc3f4cb3f5f18d3b8cafdcbf5f30105b35919f4e83697c71e838ed71493
2020-02-19 08:20:38 -08:00
practicalswift
f2472f6460 tests: Improve test runner output in case of target errors 2020-02-19 14:27:19 +00:00
practicalswift
733bbec34f tests: Add --exclude integer,parse_iso8601 (temporarily) to make Travis pass until uninitialized read issue in FormatISO8601DateTime is fixed 2020-02-19 14:11:54 +00:00
practicalswift
5ea81449f3 tests: Add support for excluding fuzz targets using -x/--exclude 2020-02-19 14:10:22 +00:00
practicalswift
555236f769 tests: Remove -detect_leaks=0 from test/fuzz/test_runner.py - no longer needed 2020-02-19 13:36:03 +00:00
0xb10c
d6d2602a32 add: test that transactions expire from mempool
This tests that a mempool transaction expires after a given timeout
and its children are removed as well.

Both the default expiry timeout defied by DEFAULT_MEMPOOL_EXPIRY and
a user definable expiry timeout via the -mempoolexpiry=<n> command
line argument (<n> is the timeout in hours) are tested.
2020-02-19 10:03:48 +01:00
Samuel Dobson
68e841e0af
Merge #18067: wallet: Improve LegacyScriptPubKeyMan::CanProvide script recognition
a304a3632f Revert "Store p2sh scripts in AddAndGetDestinationForScript" (Russell Yanofsky)
eb7d8a5b07 [test] check for addmultisigaddress regression (Sjors Provoost)
005f8a92cc wallet: Improve LegacyScriptPubKeyMan::CanProvide script recognition (Russell Yanofsky)

Pull request description:

  Make `LegacyScriptPubKeyMan::CanProvide` method able to recognize p2sh scripts when the redeem script is present in the `mapScripts` map without the p2sh script also having to be added to the `mapScripts` map. This restores behavior prior to #17261, which I think broke backwards compatibility with old wallet files by no longer treating addresses created by `addmultisigaddress` calls before #17261 as solvable.

  The reason why tests didn't fail with the CanProvide implementation in #17261 is because of a workaround added in 4a7e43e846 "Store p2sh scripts in AddAndGetDestinationForScript", which masked the problem for new `addmultisigaddress` RPC calls without fixing it for multisig addresses already created in old wallet files.

  This change adds a lot of comments and allows reverting commit 4a7e43e846 "Store p2sh scripts in AddAndGetDestinationForScript", so the `AddAndGetDestinationForScript()` function, `CanProvide()` method, and `mapScripts` map should all be more comprehensible

ACKs for top commit:
  Sjors:
    re-ACK a304a3632f (rebase, slight text changes and my test)
  achow101:
    re-ACK a304a3632f
  meshcollider:
    utACK a304a3632f

Tree-SHA512: 03b625220c49684c376a8062d7646aeba0e5bfe043f977dc7dc357a6754627d594e070e4d458d12d2291888405d94c1dbe08c7787c318374cedd5755e724fb6e
2020-02-19 14:28:41 +13:00
practicalswift
a3b539a924 ci: Run fuzz testing test cases under valgrind 2020-02-18 06:56:26 +00:00
MarcoFalke
36f42e1bf4
Merge #18037: Util: Allow scheduler to be mocked
8bca30ea17 [rpc] expose ability to mock scheduler via the rpc (Amiti Uttarwar)
7c8b6e5b52 [lib] add scheduler to node context (Amiti Uttarwar)
930d837542 [test] add chainparams property to indicate chain allows time mocking (Amiti Uttarwar)
1cd43e83c6 [test] unit test for new MockForward scheduler method (Amiti Uttarwar)
a6f63598ad [util] allow scheduler to be mocked (Amiti Uttarwar)

Pull request description:

  This PR is to support functional tests by allowing the scheduler to be mocked via the RPC.

  It adds a `MockForward` method to the scheduler class that iterates through the task queue and reschedules them to be `delta_seconds` sooner.

  This is currently used to support functional testing of the "unbroadcast" set tracking in #18038. If this patch is accepted, it would also be useful to simplify the code in #16698.

ACKs for top commit:
  MarcoFalke:
    ACK 8bca30ea17, only change is some style fixups 🕓

Tree-SHA512: 2a97fe8ade2b7fd1fb5cdfa1dcafb3227a377d7a847e3845a228bc119eb77824b4aefa43d922a06d583939b22725e223f308cf092961048079d36f6b1d9a639b
2020-02-17 17:01:50 -08:00
Amiti Uttarwar
8bca30ea17 [rpc] expose ability to mock scheduler via the rpc 2020-02-17 14:49:34 -08:00
Amiti Uttarwar
7c8b6e5b52 [lib] add scheduler to node context
- also update test setup & access point in denial of service test
2020-02-17 14:49:34 -08:00
Samer Afach
be94096dfb Fix a violation of C++ standard rules that unions cannot be switched. 2020-02-17 20:53:50 +01:00
Wladimir J. van der Laan
179504ccb6
Merge #17948: build: pass -fno-ident in Windows gitian descriptor
530d02addb build: pass -fno-ident in Windows gitian descriptor (fanquake)

Pull request description:

  `-fno-ident` prevents compilers from emitting compiler name and version number information that can needlessly bloat binaries.

  For example, in the `v0.19.0.1` Windows release binaries, there are > 1000 GCC compiler version strings embedded:
  ```bash
  # GCC: (GNU) 7.3-posix 20180312... & GCC: (GNU) 6.3.0 20170415.......
  strings bitcoind.exe | rg GCC | wc -l
      1021
  ```

  They end up collected in the end of the`.rdata` section, and cannot be removed by `strip`. i.e:

  ```bash
  objdump --section=.rdata --full-contents bitcoind.exe
  ...
   cfcc00 00000000 00000000 00000000 00000000  ................
   cfcc10 00000000 00000000 00000000 00000000  ................
   cfcc20 4743433a 2028474e 55292036 2e332e30  GCC: (GNU) 6.3.0
   cfcc30 20323031 37303431 35000000 00000000   20170415.......
   cfcc40 4743433a 2028474e 55292037 2e332d70  GCC: (GNU) 7.3-p
   cfcc50 6f736978 20323031 38303331 32000000  osix 20180312...
   cfcc60 4743433a 2028474e 55292037 2e332d70  GCC: (GNU) 7.3-p
   cfcc70 6f736978 20323031 38303331 32000000  osix 20180312...
  ```

  The flag is available for [Clang](https://clang.llvm.org/docs/ClangCommandLineReference.html#cmdoption-clang-qn) and [GCC](https://gcc.gnu.org/onlinedocs/gcc/Code-Gen-Options.html#index-fno-ident).

  Relevant code in [GCC](https://github.com/gcc-mirror/gcc/blob/master/gcc/toplev.c#L565-L578):
  ```c
    /* Attach a special .ident directive to the end of the file to identify
       the version of GCC which compiled this code.  The format of the .ident
       string is patterned after the ones produced by native SVR4 compilers.  */
    if (!flag_no_ident)
      {
        const char *pkg_version = "(GNU) ";
        char *ident_str;

        if (strcmp ("(GCC) ", pkgversion_string))
  	pkg_version = pkgversion_string;

        ident_str = ACONCAT (("GCC: ", pkg_version, version_string, NULL));
        targetm.asm_out.output_ident (ident_str);
      }
  ```

ACKs for top commit:
  practicalswift:
    ACK 530d02addb
  laanwj:
    ACK 530d02addb

Tree-SHA512: b3b28f43ec483dee28d1df8548fe72425bf00e750701825c256395f6aa7b23256eb27609b51779b86aed108b6eaa3912181a9d8282e23eebf9cee7784f9fabe0
2020-02-17 12:05:44 +01:00
Wladimir J. van der Laan
051439813e
Merge #13339: wallet: Replace %w by wallet name in -walletnotify script
4e9efac678 test: Check wallet name in -walletnotify script (João Barbosa)
9a5b5ee81f wallet: Replace %w by wallet name in -walletnotify script (João Barbosa)

Pull request description:

  Fixes #13237.

ACKs for top commit:
  laanwj:
    ACK 4e9efac678

Tree-SHA512: 189dd1c785485f2e974d7c12531851b2a977778b3b954aa95efd527322ba3345924cfd587fb9c90b0fa979202af0ab2d90e53d125fe266a36c94f757e4176203
2020-02-17 11:59:23 +01:00
MarcoFalke
263f53e2d0
Merge #18098: scripted-diff: Add missing spaces in RPCResult, Normalize type names
fad027fb0c scripted-diff: Add missing spaces in RPCResult, Fix type names (MarcoFalke)

Pull request description:

  This makes the rendered diff smaller when the RPCResult is machine generated later on (Previous attempts: #14601 and #14459)

ACKs for top commit:
  Sjors:
    ACK fad027fb0c

Tree-SHA512: 48afd571b1cd349ca0b29bb444c1c7cda657e07dd96c610d479f931ccd938186aec98e533d0552b5b10afc9a3d7b911359260a49448e8e1106e3647b2c71f3ba
2020-02-16 17:26:21 -08:00