Commit graph

471 commits

Author SHA1 Message Date
MarcoFalke
faa7d8a3f7
util: Add SaturatingAdd helper 2022-02-21 14:32:53 +01:00
MarcoFalke
abaf943477
Merge bitcoin/bitcoin#24231: streams: Fix read-past-the-end and integer overflows
fa1b89a6bd scripted-diff: Rename nReadPos to m_read_pos in streams.h (MarcoFalke)
fa56c79df9 Make CDataStream work properly on 64-bit systems (MarcoFalke)
fab02f7991 streams: Fix read-past-the-end and integer overflows (MarcoFalke)

Pull request description:

  This is a follow-up to commit e26b62093a with the following fixes:

  * Fix unsigned integer overflow in `ignore()`, when `nReadPos` wraps.
  * Fix unsigned integer overflow in `read()`, when `nReadPos` wraps.
  * Fix read-past-the-end in `read()`, when `nReadPos` wraps.

  This shouldn't be remote-exploitable, because it requires a stream of more than 1GB of size. However, it might be exploitable if the attacker controls the datadir (I haven't checked).

  A unit test for the overflow in `ignore()` looks like following. It is left as an excercise to the reader to replace `foo.ignore(7)` with the appropriate call to `read()` to reproduce the overflow and read-error in `read()`.

  ```diff
  diff --git a/src/test/coins_tests.cpp b/src/test/coins_tests.cpp
  index 922fd8e513..ec6ea93919 100644
  --- a/src/test/coins_tests.cpp
  +++ b/src/test/coins_tests.cpp
  @@ -534,6 +534,20 @@ BOOST_AUTO_TEST_CASE(ccoins_serialization)
       } catch (const std::ios_base::failure&) {
       }

  +    CDataStream foo{0, 0};
  +    auto size{std::numeric_limits<uint32_t>::max()};
  +    foo.resize(size);
  +    BOOST_CHECK_EQUAL(foo.size(), size);
  +    foo.ignore(std::numeric_limits<int32_t>::max());
  +    size -= std::numeric_limits<int32_t>::max();
  +    BOOST_CHECK_EQUAL(foo.size(), size);
  +    foo.ignore(std::numeric_limits<int32_t>::max());
  +    size -= std::numeric_limits<int32_t>::max();
  +    BOOST_CHECK_EQUAL(foo.size(), size);
  +    BOOST_CHECK_EQUAL(foo.size(), 1);
  +    foo.ignore(7); // Should overflow, as the size is only 1
  +    BOOST_CHECK_EQUAL(foo.size(), uint32_t(1 - 7));
  +
       // Very large scriptPubKey (3*10^9 bytes) past the end of the stream
       CDataStream tmp(SER_DISK, CLIENT_VERSION);
       uint64_t x = 3000000000ULL;
  ```

ACKs for top commit:
  klementtan:
    Code Review ACK fa1b89a6bd:

Tree-SHA512: 67f0a1baafe88eaf1dc844ac55b638d5cf168a18c945e3bf7a2cb03c9a5976674a8e3af2487d8a2c3eae21e5c0e7a519c8b16ee7f104934442e2769d100660e9
2022-02-21 08:09:18 +01:00
laanwj
6c4fd36089 util: Add missing rseq to syscall sandbox
Fixes #24368.
2022-02-17 15:01:43 +01:00
laanwj
df0825046a
Merge bitcoin/bitcoin#24331: util: Revert back MoveFileExW call for MinGW-w64
dc01cbc538 test: Add fs_tests/rename unit test (Hennadii Stepanov)
d4999d40b9 util: Revert back MoveFileExW call for MinGW-w64 (Hennadii Stepanov)

Pull request description:

  Unfortunately, bitcoin/bitcoin#24308 introduced a [regression](https://github.com/bitcoin/bitcoin/pull/24308#issuecomment-1037259386) for mingw builds.

  The root of the problem is a broken implementation of [`std::filesystem::rename`](https://en.cppreference.com/w/cpp/filesystem/rename). In particular, the expected behavior
  > If `old_p` is a non-directory file, then `new_p` must be ... existing non-directory file: `new_p` _is first deleted_...

  fails with the "File exists" error.

  This PR reverts back the `MoveFileExW` call, and adds the [suggested](https://github.com/bitcoin/bitcoin/pull/24308#pullrequestreview-878832906) unit test.

ACKs for top commit:
  vasild:
    ACK dc01cbc538

Tree-SHA512: c8e5a98844cfa32bec0ad67a1aaa58fe2efd0c5474d3e83490211985b110f83245758a742dcaa0a933a192ab66a7f11807e0c53ae69260b7dd02fc99f6d03849
2022-02-17 12:29:41 +01:00
MarcoFalke
fa455975e5
util: Add missing unlinkat to syscall sandbox 2022-02-14 17:12:34 +01:00
Hennadii Stepanov
d4999d40b9
util: Revert back MoveFileExW call for MinGW-w64 2022-02-14 16:56:35 +02:00
Vasil Dimov
ee822d85d6
util: use stronger-guarantee rename method
Use std::filesystem::rename() instead of std::rename(). We rely on the
destination to be overwritten if it exists, but std::rename()'s behavior
is implementation-defined in this case.
2022-02-10 08:16:05 +00:00
fanquake
243a9c3925
Merge bitcoin/bitcoin#24297: Fix unintended unsigned integer overflow in strencodings
fac9fe5d05 Fix unintended unsigned integer overflow in strencodings (MarcoFalke)

Pull request description:

  This fixes two issues for strings that start with a colon and only have one colon:

  * `fMultiColon` is incorrectly set to `true`
  * There is an unsigned integer overflow `colon - 1` (`0 - 1`)

  Neither issue matters, as the result is discarded. Though, it makes sense to still fix the issue for clarity and to avoid sanitizer issues in the function.

ACKs for top commit:
  laanwj:
    Code review ACK fac9fe5d05
  shaavan:
    Code Review ACK fac9fe5d05

Tree-SHA512: e71c21a0b617abf241e561ce6b90b963e2d5e2f77bd9547ce47209a1a94b454384391f86ef5d35fedd4f4df19add3896bb3d61fed396ebba8e864e3eeb75ed59
2022-02-10 07:17:32 +00:00
fanquake
3dc0bb9552
Merge bitcoin/bitcoin#24298: fuzz: Avoid unsigned integer overflow in FormatParagraph
fa2f7d0059 fuzz: Avoid unsigned integer overflow in FormatParagraph (MarcoFalke)

Pull request description:

  `FormatParagraph` is only ever called with compile time constant arguments, so I don't see the need for fuzzing it.

  Though, keep it for now, but avoid the unsigned integer overflow with this patch.

ACKs for top commit:
  laanwj:
    Code review ACK fa2f7d0059

Tree-SHA512: 01fc64a9ef73c183921ca1b0cd8db9514c0a242e3acf215a3393f383ae129e01625ebb16eaf9cb86370eda62d0145c3dcf8f62e40edf5958abc1f777c5687280
2022-02-10 07:14:24 +00:00
Hennadii Stepanov
ebda2b8c81
util: Drop no longer needed StripRedundantLastElementsOfPath() function 2022-02-09 19:33:24 +02:00
Hennadii Stepanov
06fed4c21e
Use ArgsManager::GetPathArg() for "-blocksdir" option 2022-02-09 19:31:23 +02:00
Hennadii Stepanov
15b632bf16
Use ArgsManager::GetPathArg() for "-datadir" option 2022-02-09 19:31:22 +02:00
Hennadii Stepanov
540ca5111f
util: Add ArgsManager::GetPathArg() function
Co-authored-by: Ryan Ofsky <ryan@ofsky.org>
2022-02-09 19:31:16 +02:00
MarcoFalke
fab02f7991
streams: Fix read-past-the-end and integer overflows 2022-02-09 17:20:22 +01:00
MarcoFalke
fa2f7d0059
fuzz: Avoid unsigned integer overflow in FormatParagraph 2022-02-09 14:38:22 +01:00
MarcoFalke
fac9fe5d05
Fix unintended unsigned integer overflow in strencodings 2022-02-09 13:24:55 +01:00
MarcoFalke
8edb0416dd
Merge bitcoin/bitcoin#24266: util: Avoid buggy std::filesystem:::create_directories() call
b9c113af75 util: Avoid buggy std::filesystem:::create_directories() call (Hennadii Stepanov)

Pull request description:

  Compiled with some libstdc++ versions (e.g., on Ubuntu 20.04) [`std::filesystem:::create_directories()`](https://en.cppreference.com/w/cpp/filesystem/create_directory) call [fails](https://github.com/bitcoin/bitcoin/issues/24257#issue-1123753243) to handle symbol links properly.

  No behavior change in comparison to the [pre-20744](c194293883) master branch.

  Fixes bitcoin/bitcoin#24257.

ACKs for top commit:
  ryanofsky:
    Code review ACK b9c113af75. Nice simplification and fix
  MarcoFalke:
    review ACK b9c113af75 🐬

Tree-SHA512: 79d940cfc1f68d9b0548fb2ab005e90850b54ac0fb3bb2940afd632d56288d92687579a3176bac3fd0ea3d2dae71e26444f8f7bdb87862414c12866ae5e857c4
2022-02-08 15:46:34 +01:00
Hennadii Stepanov
b9c113af75
util: Avoid buggy std::filesystem:::create_directories() call
Compiled with some libstdc++ versions (e.g., on Ubuntu 20.04)
std::filesystem:::create_directories() call fails to handle symbol links
properly.
2022-02-05 18:32:39 +02:00
Ryan Ofsky
80cd64e842 Re-enable util_datadir check disabled in #20744
This should also fix an assert error if a -datadir with a trailing slash
is used on windows. This appears to be a real error and regression
introduced with #20744.

On windows (or at least wine), fs calls that actuallly access the
filesystem like fs::equivalent or fs::exists seem to treat directory
paths with trailing slashes as not existing, so it's necessary to
normalize these paths before using them. This fix adds a
path::lexically_normal() call to the failing assert so it passes.
2022-02-04 09:09:09 -05:00
Kiminuo
41d7166c8a
refactor: replace boost::filesystem with std::filesystem
Warning: Replacing fs::system_complete calls with fs::absolute calls
in this commit may cause minor changes in behaviour because fs::absolute
no longer strips trailing slashes; however these changes are believed to
be safe.

Co-authored-by: Russell Yanofsky <russ@yanofsky.org>
Co-authored-by: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com>
2022-02-03 18:35:52 +08:00
fanquake
21f781ad79
fs: consistently use fsbridge for {i,o}fstream
Part of #20744, but this can be done now, and will simplify the diff.
2022-01-26 22:08:19 +08:00
fanquake
63fc2f5cce
Merge bitcoin/bitcoin#24065: build: explicitly disable support for external signing on Windows
e2ab9f83f8 build: disable external signer on Windows (fanquake)

Pull request description:

  This change explicitly disables support for external signing when targeting Windows and OpenBSD. The driver for this is that Boost Process uses boost::filesystem internally, when targeting Windows, which gets in the way of removing our usage of it (#20744). While we could adjust #20744 to still link against the Boost libs when building for Windows, that would be disappointing, as we wouldn't have cleanly removed the Boost usage we're trying too (including the build infrastructure), and, we'd be in a position where we would be building releases differently depending on the platform, which is something I want to avoid.

  After discussion with Sjors, Achow and Hebasto, this seemed like a reasonable step to move #20744 forward (as-is). Note that support for external signing ([while already being experimental](https://github.com/bitcoin/bitcoin/blob/master/doc/external-signer.md#example-usage)), could be considered even more experimental on Windows. Also, oddly, we have external-signing [explicitly disabled in our Windows (cross-compile) CI](807169e10b/ci/test/00_setup_env_win64.sh (L16)), it's not clear why this is the case, as, if it's a feature being built into releases, it should be being built and tested in the CI which is most-like the release process.

  There is an [issue open upstream](https://github.com/boostorg/process/issues/207), in regards to migrating Boost Process to std::filesystem, or having an option to use it. However there hasn't been much discussion since it was opened ~9 months ago. There is another related issue here: https://github.com/klemens-morgenstern/boost-process/issues/164.

  Resolves #24036.

ACKs for top commit:
  Sjors:
    utACK e2ab9f8
  achow101:
    ACK e2ab9f83f8
  kallewoof:
    utACK e2ab9f83f8
  hebasto:
    ACK e2ab9f83f8, tested on Linux Mint 20.2 (x86_64).

Tree-SHA512: 36fcfc0e1a008a8271dc76b8e12e93d3e1d1e528bf668e95a559e9f6fd7d5f031bd7a6a6bc8b9fa9d057b2cd56f9ec8838c7f74e87899bf9a6aeb787afbd112c
2022-01-20 13:13:30 +08:00
Hennadii Stepanov
f251141483
util: Fix mis-swapped prettyIndent and indentLevel arguments 2022-01-18 15:47:46 +02:00
fanquake
e2ab9f83f8
build: disable external signer on Windows 2022-01-15 10:02:04 +08:00
James O'Beirne
b5c9bb5cb9 util: Restore GetIntArg saturating behavior
The new locale-independent atoi64 method introduced in #20452 parses
large integer values higher than maximum representable value as 0
instead of the maximum value, which breaks backwards compatibility.
This commit restores compatibility and adds test coverage for this case
in terms of the related GetIntArg and strtoll functions.

Co-authored-by: Ryan Ofsky <ryan@ofsky.org>
2022-01-11 19:54:36 -05:00
Pieter Wuille
efab28b06b Add FastRange32 function and use it throughout the codebase 2022-01-07 13:37:47 -05:00
Pieter Wuille
96ecd6fa3e scripted-diff: rename MapIntoRange to FastRange64
-BEGIN VERIFY SCRIPT-
sed -i -e 's/MapIntoRange/FastRange64/' src/blockfilter.cpp src/test/fuzz/golomb_rice.cpp src/util/fastrange.h
-END VERIFY SCRIPT-
2022-01-06 11:29:55 -05:00
Pieter Wuille
c6d15c45d9 [moveonly] Move MapIntoRange() to separate util/fastrange.h 2022-01-06 11:27:06 -05:00
MarcoFalke
70395bab4e
Merge bitcoin/bitcoin#23760: util: move MapIntoRange() for reuse in fuzz tests
df2307cdc3 util: move MapIntoRange() for reuse in fuzz tests (fanquake)

Pull request description:

ACKs for top commit:
  shaavan:
    ACK df2307cdc3

Tree-SHA512: 31bf18f50a82e442ff025d6be0db5666b463a1fc16ec6b2112c77bb815515d27f8a537a0c9934c7daa3f4d526b47e8d6333f75a13b271e6efa550f8e71504b0a
2022-01-06 14:54:12 +01:00
W. J. van der Laan
8f1c28a609
Merge bitcoin/bitcoin#21879: refactor: wrap accept() and extend usage of Sock
6bf6e9fd9d net: change CreateNodeFromAcceptedSocket() to take Sock (Vasil Dimov)
9e3cbfca7c net: use Sock in CConnman::ListenSocket (Vasil Dimov)
f8bd13f85a net: add new method Sock::Accept() that wraps accept() (Vasil Dimov)

Pull request description:

  _This is a piece of https://github.com/bitcoin/bitcoin/pull/21878, chopped off to ease review._

  Introduce an `accept(2)` wrapper `Sock::Accept()` and extend the usage of `Sock` in `CConnman::ListenSocket` and `CreateNodeFromAcceptedSocket()`.

ACKs for top commit:
  laanwj:
    Code review ACK 6bf6e9fd9d
  jamesob:
    ACK 6bf6e9fd9d ([`jamesob/ackr/21879.2.vasild.wrap_accept_and_extend_u`](https://github.com/jamesob/bitcoin/tree/ackr/21879.2.vasild.wrap_accept_and_extend_u))
  jonatack:
    ACK 6bf6e9fd9d per `git range-diff ea989de 976f6e8 6bf6e9f` -- only change since my last review was `s/listen_socket.socket/listen_socket.sock->Get()/` in `src/net.cpp: CConnman::SocketHandlerListening()` -- re-read the code changes, rebase/debug build/ran units following my previous full review (https://github.com/bitcoin/bitcoin/pull/21879#pullrequestreview-761251278)
  w0xlt:
    tACK 6bf6e9f

Tree-SHA512: dc6d1acc4f255f1f7e8cf6dd74e97975cf3d5959e9fc2e689f74812ac3526d5ee8b6a32eca605925d10a4f7b6ff1ce5e900344311e587d19786b48c54d021b64
2022-01-05 15:32:53 +01:00
MarcoFalke
e31cdb0238
Merge bitcoin/bitcoin#23411: refactor: Avoid integer overflow in ApplyStats when activating snapshot
fa996c58e8 refactor: Avoid integer overflow in ApplyStats when activating snapshot (MarcoFalke)
fac01888d1 Move AdditionOverflow to util, Add CheckedAdd with unit tests (MarcoFalke)
fa526d8fb6 Add dev doc to CCoinsStats::m_hash_type and make it const (MarcoFalke)
faff051560 style: Remove unused whitespace (MarcoFalke)

Pull request description:

  A snapshot contains the utxo set, including the out value. To activate the snapshot, the hash needs to be calculated. As a side-effect, the total amount in the snapshot is calculated (as the sum of all out values), but never used. Instead of running into an integer overflow in an unused result, don't calculate the result in the first place.

  Other code paths (using the active utxo set) can not run into an integer overflow, since the active utxo set is valid.

  Fixes https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=39716

ACKs for top commit:
  shaavan:
    reACK fa996c58e8
  vasild:
    ACK fa996c58e8

Tree-SHA512: 4f207f634841f6f634fd02ae1e5907e343fd767524fd0e8149aa99fa9a1834fe50167d14874834d45236e9c325d567925f28129bacb7d80be29cf22277a16a14
2022-01-05 10:34:29 +01:00
Hennadii Stepanov
f47dda2c58
scripted-diff: Bump copyright headers
-BEGIN VERIFY SCRIPT-
./contrib/devtools/copyright_header.py update ./
-END VERIFY SCRIPT-

Commits of previous years:
* 2020: fa0074e2d8
* 2019: aaaaad6ac9
2021-12-30 19:36:57 +02:00
fanquake
df2307cdc3
util: move MapIntoRange() for reuse in fuzz tests 2021-12-23 15:05:22 +08:00
MarcoFalke
fac01888d1
Move AdditionOverflow to util, Add CheckedAdd with unit tests 2021-12-17 10:46:39 +01:00
MarcoFalke
fa5865a9e3
Reduce size of strencodings decode tables 2021-12-13 09:58:20 +01:00
MarcoFalke
fad6761cf7
Fix implicit integer sign changes in strencodings 2021-12-13 09:57:33 +01:00
MarcoFalke
fa77f95c2f
fuzz: Fix RPC internal bug detection 2021-12-08 14:20:16 +01:00
Vasil Dimov
f8bd13f85a
net: add new method Sock::Accept() that wraps accept()
This will help to increase `Sock` usage and make more code mockable.
2021-12-01 15:22:08 +01:00
W. J. van der Laan
cf24152596
Merge bitcoin/bitcoin#21206: refactor: Make CWalletTx sync state type-safe
d8ee8f3cd3 refactor: Make CWalletTx sync state type-safe (Russell Yanofsky)

Pull request description:

  Current `CWalletTx` state representation makes it possible to set inconsistent states that won't be handled correctly by wallet sync code or serialized & deserialized back into the same form.

  For example, it is possible to call `setConflicted` without setting a conflicting block hash, or `setConfirmed` with no transaction index. And it's possible update individual `m_confirm` and `fInMempool` data fields without setting an overall consistent state that can be serialized and handled correctly.

  Fix this without changing behavior by using `std::variant`, instead of an enum and collection of fields, to represent sync state, so state tracking code is safer and more legible.

  This is a first step to fixing state tracking bugs https://github.com/bitcoin-core/bitcoin-devwiki/wiki/Wallet-Transaction-Conflict-Tracking, by adding an extra margin of safety that can prevent new bugs from being introduced as existing bugs are fixed.

ACKs for top commit:
  laanwj:
    re-ACK d8ee8f3cd3
  jonatack:
    Code review ACK d8ee8f3cd3

Tree-SHA512: b9f15e9d99dbdbdd3ef7a76764e11f66949f50e6227e284126f209e4cb106af6d55e9a9e8c7d4aa216ddc92c6d5acc6f4aa4746f209bbd77f03831b51a2841c3
2021-11-25 19:41:53 +01:00
MarcoFalke
9394964f6b
Merge bitcoin/bitcoin#23451: span: Add std::byte helpers
faa3ec2304 span: Add std::byte helpers (MarcoFalke)
fa18038f51 refactor: Use ignore helper when unserializing an invalid pubkey (MarcoFalke)
fabe18d0b3 Use value_type in CDataStream where possible (MarcoFalke)

Pull request description:

  This adds (currently unused) span std::byte helpers, so that they can be used in new code.

  The refactors are also required for https://github.com/bitcoin/bitcoin/pull/23438, but they are split up because the other pull doesn't compile with msvc right now.

  The third commit is not needed for the other pull, but still nice.

ACKs for top commit:
  klementtan:
    reACK  faa3ec2. Verified that all the new `std::byte` helper functions are tested.
  laanwj:
    Code review ACK faa3ec2304

Tree-SHA512: b1f6af39f03ea4dfebf20d4a8538fa993a6104e7fc92ddf0c4606a7efc3ca9a8c1a4741d98a1418569c11bb9ce9258bf0c0c06d93d85ed7e208902a2db04e407
2021-11-24 11:04:37 +01:00
MarcoFalke
73ac195e29
Merge bitcoin/bitcoin#23249: util: ParseByteUnits - Parse a string with suffix unit
21b58f430f util: ParseByteUnits - Parse a string with suffix unit [k|K|m|M|g|G|t|T] (Douglas Chimento)

Pull request description:

  A convenience utility for parsing human readable strings sizes e.g. `500G` is `500 * 1 << 30`

  The argument/setting `maxuploadtarget`  now accept human readable byte units `[k|K|m|M|g|G||t|T]`
  This change  backward compatible, defaults to `M` if no unit specified.

ACKs for top commit:
  vasild:
    ACK 21b58f430f
  ryanofsky:
    Code review ACK 21b58f430f. Only changes since last review are dropping optional has_value call, fixing comment punctuation, squashing commits.

Tree-SHA512: c9b85acc0f77c847a0290b27ac5dc586ecc078110cf133063140576a04c11aa9c553159b9b4993488edcf6e60db6837de7c83b2964639bc21e8ffa4d455a5eb7
2021-11-24 10:49:13 +01:00
MarcoFalke
fade4b3aca
util: Add missing fstatfs to syscall sandbox 2021-11-19 17:18:59 +01:00
Douglas Chimento
21b58f430f
util: ParseByteUnits - Parse a string with suffix unit [k|K|m|M|g|G|t|T]
A convenience utility for human readable arguments/config e.g. -maxuploadtarget=500g
2021-11-17 12:47:30 +02:00
MarcoFalke
fa44237d76
doc: Fix typos in endif header comments 2021-11-16 09:56:45 +01:00
Russell Yanofsky
d8ee8f3cd3 refactor: Make CWalletTx sync state type-safe
Current CWalletTx state representation makes it possible to set
inconsistent states that won't be handled correctly by wallet sync code
or serialized & deserialized back into the same form.

For example, it is possible to call setConflicted without setting a
conflicting block hash, or setConfirmed with no transaction index. And
it's possible update individual m_confirm and fInMempool data fields
without setting an overall consistent state that can be serialized and
handled correctly.

Fix this without changing behavior by using std::variant, instead of an
enum and collection of fields, to represent sync state, so state
tracking code is safer and more legible.

This is a first step to fixing state tracking bugs
https://github.com/bitcoin-core/bitcoin-devwiki/wiki/Wallet-Transaction-Conflict-Tracking,
by adding an extra margin of safety that can prevent new bugs from being
introduced as existing bugs are fixed.
2021-11-15 09:11:44 -05:00
MarcoFalke
38b2a0a3f9
Merge bitcoin/bitcoin#23173: Add ChainstateManager::ProcessTransaction
0fdb619aaf [validation] Always call mempool.check() after processing a new transaction (John Newbery)
2c64270bbe [refactor] Don't call AcceptToMemoryPool() from outside validation.cpp (John Newbery)
92a3aeecf6 [validation] Add CChainState::ProcessTransaction() (John Newbery)
36167faea9 [logging/documentation] Remove reference to AcceptToMemoryPool from error string (John Newbery)
4c24142b1e [validation] Remove comment about AcceptToMemoryPool() (John Newbery)
5759fd12b8 [test] Don't set bypass_limits to true in txvalidation_tests.cpp (John Newbery)
497c9e2964 [test] Don't set bypass_limits to true in txvalidationcache_tests.cpp (John Newbery)

Pull request description:

  Similarly to how #18698 added `ProcessNewBlock()` and `ProcessNewBlockHeaders()` methods to the `ChainstateManager` class, this PR adds a new `ProcessTransaction()` method. Code outside validation no longer calls `AcceptToMemoryPool()` directly, but calls through the higher-level `ProcessTransaction()` method. Advantages:

  - The interface is simplified. Calling code no longer needs to know about the active chainstate or mempool object, since `AcceptToMemoryPool()` can only ever be called for the active chainstate, and that chainstate knows which mempool it's using. We can also remove the `bypass_limits` argument, since that can only be used internally in validation.
  - responsibility for calling `CTxMemPool::check()` is removed from the callers, and run automatically by `ChainstateManager` every time `ProcessTransaction()` is called.

ACKs for top commit:
  lsilva01:
    tACK 0fdb619 on Ubuntu 20.04
  theStack:
    Code-review ACK 0fdb619aaf
  ryanofsky:
    Code review ACK 0fdb619aaf. Only changes since last review: splitting & joining commits, adding more explanations to commit messages, tweaking MEMPOOL_ERROR string, fixing up argument name comments.

Tree-SHA512: 0b395c2e3ef242f0d41d47174b1646b0a73aeece38f1fe29349837e6fb832f4bf8d57e1a1eaed82a97c635cfd59015a7e07f824e0d7c00b2bee4144e80608172
2021-11-10 14:35:22 +01:00
MarcoFalke
faa3ec2304
span: Add std::byte helpers
Also, add Span<std::byte> interface to strencondings.
2021-11-09 17:42:13 +01:00
John Newbery
36167faea9 [logging/documentation] Remove reference to AcceptToMemoryPool from error string
User-facing error messages should not leak internal implementation
details like function names. Update the MEMPOOL_REJECTED error string
from "Transaction rejected by AcceptToMemoryPool" to the more generic
"Transaction rejected by mempool". Also update the MEMPOOL_ERROR error
message from "AcceptToMemoryPool failed" to the more precise "Mempool
internal error" since this error indicates and internal (e.g.
logic/hardware/etc) failure, and not a transaction rejection.
2021-11-03 14:28:04 +00:00
Russell Yanofsky
c5d7e34bd9 scripted-diff: disable unimplemented ArgsManager BOOL/INT/STRING flags
This commit does not change behavior in any way. See previous commit for
complete rationale, but these flags are being disabled because they
aren't implemented and will otherwise break backwards compatibility when
they are implemented.

-BEGIN VERIFY SCRIPT-
sed -i 's:\(ALLOW_.*\)   \(//!< unimplemented\):// \1\2:' src/util/system.h
sed -i '/DISALLOW_NEGATION.*scripted-diff/d' src/util/system.cpp
git grep -l 'ArgsManager::ALLOW_\(INT\|STRING\)'  | xargs sed -i 's/ArgsManager::ALLOW_\(INT\|STRING\)/ArgsManager::ALLOW_ANY | ArgsManager::DISALLOW_NEGATION/g'
git grep -l 'ALLOW_BOOL' -- ':!src/util/system.h' | xargs sed -i 's/ALLOW_BOOL/ALLOW_ANY/g'
-END VERIFY SCRIPT-
2021-10-25 10:44:17 -04:00
Russell Yanofsky
b8c069b7a9 refactor: Add explicit DISALLOW_NEGATION ArgsManager flag to clarify flag usage
Currently, ALLOW_{INT|BOOL|STRING} flags don't do any real validation,
so current uses of these flags are misleading and will also break
backwards compatibility whenever these flags are implemented in a future
PR (draft PR is #16545).

An additional complication is that while these flags don't do any real
settings validation, they do affect whether setting negation syntax is
allowed.

Fix this mess by disabling ALLOW_{INT|BOOL|STRING} flags until they are
implemented, and adding an unambiguous DISALLOW_NEGATION flag. This is
done in two commits, with this commit adding the DISALLOW_NEGATION flag,
and the next commit disabling the ALLOW_{INT|BOOL|STRING} flags.
2021-10-25 10:44:17 -04:00