Commit graph

410 commits

Author SHA1 Message Date
MarcoFalke
fab30b61eb
util: Remove unused ParseMoney that takes a c_str 2020-02-29 00:25:37 +07:00
Jeffrey Czyz
0aed17ef28 Refactor FormatStateMessage into ValidationState 2020-02-27 17:59:07 -08:00
Samuel Dobson
03f98b15ad
Merge : 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
MarcoFalke
fae86c38bc
util: Remove unused MilliSleep 2020-02-21 10:06:27 -08:00
MarcoFalke
fa4620be78 util: Add UnintrruptibleSleep 2020-02-21 09:49:37 -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
Wladimir J. van der Laan
051439813e
Merge : 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 .

ACKs for top commit:
  laanwj:
    ACK 4e9efac678

Tree-SHA512: 189dd1c785485f2e974d7c12531851b2a977778b3b954aa95efd527322ba3345924cfd587fb9c90b0fa979202af0ab2d90e53d125fe266a36c94f757e4176203
2020-02-17 11:59:23 +01:00
Jeffrey Czyz
e193a84fb2
Refactor message hashing into a utility function
And add unit test for it.

The purpose of using a preamble or "magic" text as part of signing and
verifying a message was not given when the code was repeated in a few
locations. Make a test showing how it is used to prevent inadvertently
signing a transaction.
2020-02-14 10:45:41 +01:00
Vasil Dimov
f8f0d9893d
Deduplicate the message signing code
The logic of signing a message was duplicated in 3 places:

src/qt/signverifymessagedialog.cpp
  SignVerifyMessageDialog::on_signMessageButton_SM_clicked()

src/rpc/misc.cpp
  signmessagewithprivkey()

src/wallet/rpcwallet.cpp
  signmessage()

Move the logic into

src/util/message.cpp
  MessageSign()

and call it from all the 3 places.
2020-02-14 10:45:40 +01:00
Vasil Dimov
2ce3447eb1
Deduplicate the message verifying code
The logic of verifying a message was duplicated in 2 places:

src/qt/signverifymessagedialog.cpp
  SignVerifyMessageDialog::on_verifyMessageButton_VM_clicked()

src/rpc/misc.cpp
  verifymessage()

with the only difference being the result handling. Move the logic into
a dedicated

src/util/message.cpp
  MessageVerify()

which returns a set of result codes, call it from the 2 places and just
handle the results differently in the callers.
2020-02-14 10:45:40 +01:00
Wladimir J. van der Laan
8a56f79d49
Merge : util: Disallow network-qualified command line options
900d8f6f70 util: Disallow network-qualified command line options (Russell Yanofsky)

Pull request description:

  Previously these were allowed but ignored.

  This change implements one of the settings simplifications listed in . Change includes release notes.

ACKs for top commit:
  laanwj:
    ACK 900d8f6f70

Tree-SHA512: ab020a16a86c1e8ec709fbf798d533879d32c565eceeb7eb785c33042c49c6b4d1108c5453d8166e4a2abffc2c8802fbb6d3b895e0ddeefa8f274fd647e3c8ad
2020-02-05 16:23:53 +01:00
Wladimir J. van der Laan
adea5e1b54
Merge : Fix some asmap issues
c86bc14408 Make asmap Interpret tolerant of malicious map data (Pieter Wuille)
38c2395d7a Use ASNs for mapped IPv4 addresses correctly (Pieter Wuille)
6f8c937312 Mark asmap const in statistics code (Pieter Wuille)
d58bcdc4b5 Avoid asmap copies in initialization (Pieter Wuille)

Pull request description:

  Here are a few things to improve in the asmap implementation. The first two commits are just code improvements. The last one is a bugfix (the exsting code wouldn't correctly apply ASN lookups to mapped/embedded IPv4 addresses).

ACKs for top commit:
  practicalswift:
    ACK c86bc14408 -- patch looks correct
  naumenkogs:
    utACK c86bc14
  laanwj:
    ACK c86bc14408
  jonatack:
    ACK c86bc14408 code looks correct, built/ran tests, bitcoind with -asmap pointed to asmap/demo.map

Tree-SHA512: 1036f43152754d621bfbecfd3b7c7276e4670598fcaed42a3d275e51fa2cf3653e2c9e9cfa714f6c7719362541510e92171e076ac4169b55a0cc8908b2d514c0
2020-02-05 13:59:42 +01:00
Pieter Wuille
c86bc14408 Make asmap Interpret tolerant of malicious map data 2020-01-31 14:51:37 -08:00
Larry Ruane
b951b0973c on startup, write config options to debug.log 2020-01-29 15:44:00 -07:00
Wladimir J. van der Laan
01fc5891fb
Merge : p2p: supplying and using asmap to improve IP bucketing in addrman
3c1bc40205 Add extra logging of asmap use and bucketing (Gleb Naumenko)
e4658aa8ea Return mapped AS in RPC call getpeerinfo (Gleb Naumenko)
ec45646de9 Integrate ASN bucketing in Addrman and add tests (Gleb Naumenko)
8feb4e4b66  Add asmap utility which queries a mapping (Gleb Naumenko)

Pull request description:

  This PR attempts to solve the problem explained in .
  A particular attack which encouraged us to work on this issue is explained here  [[Erebus Attack against Bitcoin Peer-to-Peer Network](https://erebus-attack.comp.nus.edu.sg/)] (by @muoitranduc)

  Instead of relying on /16 prefix to diversify the connections every node creates, we would instead rely on the (ip -> ASN) mapping, if this mapping is provided.

  A .map file can be created by every user independently based on a router dump, or provided along with the Bitcoin release. Currently we use the python scripts written by @sipa to create a .map file, which is no larger than 2MB (awesome!).

  Here I suggest adding a field to peers.dat which would represent a hash of asmap file used while serializing addrman (or 0 for /16 prefix legacy approach).
  In this case, every time the file is updated (or grouping method changed), all buckets will be re-computed.
  I believe that alternative selective re-bucketing for only updated ranges would require substantial changes.

  TODO:
  - ~~more unit tests~~
  - ~~find a way to test the code without including >1 MB mapping file in the repo.~~
  - find a way to check that mapping file is not corrupted (checksum?)
  - comments and separate tests for asmap.cpp
  - make python code for .map generation public
  - figure out asmap distribution (?)

  ~Interesting corner case: I’m using std::hash to compute a fingerprint of asmap, and std::hash returns size_t. I guess  if a user updates the OS to 64-bit, then the hash of asap will change? Does it even matter?~

ACKs for top commit:
  laanwj:
    re-ACK 3c1bc40205
  jamesob:
    ACK 3c1bc40205 ([`jamesob/ackr/16702.3.naumenkogs.p2p_supplying_and_using`](https://github.com/jamesob/bitcoin/tree/ackr/16702.3.naumenkogs.p2p_supplying_and_using))
  jonatack:
    ACK 3c1bc40205

Tree-SHA512: e2dc6171188d5cdc2ab2c022fa49ed73a14a0acb8ae4c5ffa970172a0365942a249ad3d57e5fb134bc156a3492662c983f74bd21e78d316629dcadf71576800c
2020-01-29 13:55:43 +01:00
Wladimir J. van der Laan
04f78b818f
Merge : bug-fix macos: give free bytes to F_PREALLOCATE
75163f4729 bug-fix macos: give free bytes to F_PREALLOCATE (Karl-Johan Alm)

Pull request description:

  The macos manpage for `fcntl` (for `F_PEOFPOSMODE`) states:

  > Allocate from the physical end of file.  In this case, fst_length indicates the number of newly allocated bytes desired.

  This would result in the rev files being essentially pre-allocating 2x their necessary size (this is the case for block files as well, but these are flushed down to their right sizes every time) as they would pre-allocate `pos + length` **free** bytes, rather than allocating `length` bytes after `pos`, as expected.

  Fixes .

ACKs for top commit:
  eriknylund:
    ACK 75163f4729 built locally. All tests passing. Manual test as per my previous comment above on an older commit, using an APFS unencrypted disk image with 3 GB.
  laanwj:
    code review ACK 75163f4729

Tree-SHA512: 105c8d56c20acad8febdf0583f1e5721b63376ace325a7a62c2e4b15a442c7131404ed604c32c0cda716791d7ca5aa9f5b6a774ff86e39838bc7e87ca3c42760
2020-01-22 16:48:31 +01:00
João Barbosa
9a5b5ee81f wallet: Replace %w by wallet name in -walletnotify script
Co-authored-by: Russell Yanofsky <russ@yanofsky.org>
2020-01-15 11:47:26 +00:00
Karl-Johan Alm
75163f4729
bug-fix macos: give free bytes to F_PREALLOCATE
The macos manpage for fcntl (for F_PEOFPOSMODE) states:

> Allocate from the physical end of file.  In this case, fst_length indicates the number of newly allocated bytes desired.
2020-01-15 08:56:39 +09:00
MarcoFalke
e09c701e01 scripted-diff: Bump copyright of files changed in 2020
-BEGIN VERIFY SCRIPT-
./contrib/devtools/copyright_header.py update ./
-END VERIFY SCRIPT-
2020-01-15 02:18:00 +07:00
MarcoFalke
6cbe620964 scripted-diff: Replace CCriticalSection with RecursiveMutex
-BEGIN VERIFY SCRIPT-
 # Delete outdated alias for RecursiveMutex
 sed -i -e '/CCriticalSection/d'                 ./src/sync.h
 # Replace use of outdated alias with RecursiveMutex
 sed -i -e 's/CCriticalSection/RecursiveMutex/g' $(git grep -l CCriticalSection)
-END VERIFY SCRIPT-
2020-01-15 01:43:46 +07:00
Ben Woosley
498cdbb426
Fix improper Doxygen inline comments
The proper syntax is "//!<"
http://www.doxygen.nl/manual/docblocks.html#memberdoc
2020-01-10 16:32:40 -08:00
Wladimir J. van der Laan
e7f8450357
Merge : log: Add validation interface logging
f9abf4ab6d Add logging for CValidationInterface events (Jeffrey Czyz)
6edebacb21 Refactor FormatStateMessage for clarity (Jeffrey Czyz)
72f3227c83 Format CValidationState properly in all cases (Jeffrey Czyz)
428ac70095 Add VALIDATION to BCLog::LogFlags (Jeffrey Czyz)

Pull request description:

  Add logging of `CValidationInterface` callbacks using a new `VALIDATIONINTERFACE` log flag (see ). A separate flag is desirable as the logging can be noisy and thus may need to be disabled without affecting other logging.

  This could help debug issues where there may be race conditions at play, such as .

ACKs for top commit:
  jnewbery:
    ACK f9abf4ab6d
  hebasto:
    ACK f9abf4ab6d
  ariard:
    ACK f9abf4a, only changes since 0cadb12 are replacing log indication `VALIDATIONINTERFACE` by `VALIDATION` and avoiding a forward declaration with a new include
  ryanofsky:
    Code review ACK f9abf4ab6d. Just suggested changes since last review (thanks!)

Tree-SHA512: 3e0f6e2c8951cf46fbad3ff440971d95d526df2a52a2e4d6452a82785c63d53accfdabae66b0b30e2fe0b00737f8d5cb717edbad1460b63acb11a72c8f5d4236
2020-01-09 21:05:35 +01:00
Jeffrey Czyz
6edebacb21 Refactor FormatStateMessage for clarity
All cases of CValidationState were condensed into one strprintf call.
This is no longer suitable as more cases are added (e.g., IsValid).
2020-01-03 11:38:44 -08:00
Jeffrey Czyz
72f3227c83 Format CValidationState properly in all cases
FormatStateMessage does not properly handle the case where
CValidationState::IsValid() returns true. Use "Valid" for the state in
this case.
2020-01-03 11:37:26 -08:00
Wladimir J. van der Laan
190a4051fd
Merge : net: Log to net category for exceptions in ProcessMessages
4bdd68f301 Add missing typeinfo includes (Wladimir J. van der Laan)
4d88c3dcb6 net: Log to net category for exceptions in ProcessMessages (Wladimir J. van der Laan)

Pull request description:

  Remove the forest of special exceptions based on string matching, and simply log a short message to the NET logging category when an exception happens during packet processing. It is not good to panick end users with verbose errors (let alone writing to stderr) when any peer can generate them.

ACKs for top commit:
  MarcoFalke:
    re-ACK 4bdd68f301 (only change is adding includes) 🕕
  promag:
    ACK 4bdd68f301, could squash.

Tree-SHA512: a005591a3202b005c75e01dfa54249db3992e2f9eefa8b3d9d435acf66130417716ed926ce4e045179cf43788f1abc7362d999750681a9c80b318373d611c366
2020-01-02 21:39:00 +01:00
Wladimir J. van der Laan
4bdd68f301 Add missing typeinfo includes
The use of `typeid()` for logging exception types requires this include
according to https://en.cppreference.com/w/cpp/language/typeid.
2020-01-02 20:54:15 +01:00
MarcoFalke
aaaaad6ac9
scripted-diff: Bump copyright of files changed in 2019
-BEGIN VERIFY SCRIPT-
./contrib/devtools/copyright_header.py update ./
-END VERIFY SCRIPT-
2019-12-30 10:42:20 +13:00
Russell Yanofsky
900d8f6f70 util: Disallow network-qualified command line options
Previously these were allowed but ignored.
2019-12-19 16:27:15 -05:00
MarcoFalke
6677be64f6
Merge : refactor: Settings code cleanups
e9fd366044 refactor: Remove null setting check in GetSetting() (Russell Yanofsky)
cba2710220 scripted-diff: Remove unused ArgsManager type flags in tests (Russell Yanofsky)
425bb30725 refactor: Add util_CheckValue test (Russell Yanofsky)
0fa54358b0 refactor: Add ArgsManager::GetSettingsList method (Russell Yanofsky)
3e185522ac refactor: Get rid of ArgsManagerHelper class (Russell Yanofsky)
dc0f148074 refactor: Replace FlagsOfKnownArg with GetArgFlags (Russell Yanofsky)
57e8b7a727 refactor: Clean up includeconf comments (Russell Yanofsky)
3f7dc9b808 refactor: Clean up long lines in settings code (Russell Yanofsky)

Pull request description:

  This PR doesn't change behavior. It just implements some suggestions from  and  and few other small cleanups.

ACKs for top commit:
  jnewbery:
    Code review ACK e9fd366044
  MarcoFalke:
    ACK e9fd366044 🚟

Tree-SHA512: 6e100d92c72f72bc39567187ab97a3547b3c06e5fcf1a1b74023358b8bca552124ca6a53c0ab53179b7f1329c03d9a73faaef6d73d2cd1a2321568a0286525e2
2019-12-20 03:05:28 +07:00
Gleb Naumenko
8feb4e4b66 Add asmap utility which queries a mapping
The scripts for creating a compact IP->ASN mapping are here:
https://github.com/sipa/asmap

Co-authored-by: Pieter Wuille <pieter.wuille@gmail.com>
2019-12-18 14:27:03 -05:00
practicalswift
32e27129ff util: Move TrimString(...). Introduce default pattern (trims whitespace). Add NODISCARD. 2019-12-16 22:50:49 +00:00
practicalswift
a6fc26da55 util: Don't allow DecodeBase32(...) of strings with embedded NUL characters 2019-12-16 09:04:36 +00:00
practicalswift
93cc18b0f6 util: Don't allow DecodeBase64(...) of strings with embedded NUL characters 2019-12-16 09:04:36 +00:00
practicalswift
ccc53e43c5 util: Don't allow ParseMoney(...) of strings with embedded NUL characters 2019-12-16 09:04:35 +00:00
practicalswift
d945c6f5e6 util: Don't allow base58-decoding of std::string:s containing non-base58 characters 2019-12-12 11:01:56 +00:00
fanquake
d2a3a5cadb
util: make ScheduleBatchPriority advisory only 2019-11-26 09:55:07 -05:00
Russell Yanofsky
e9fd366044 refactor: Remove null setting check in GetSetting()
Also rename the "result_complete" variable in GetSettingsList() to "done" to be
more consistent with GetSetting().

This change doesn't affect current behavior but could be useful in the future
to support dynamically changing settings at runtime and adding new settings
sources, because it lets high priority sources reset settings back to default
(see test).

By removing a special case for null, this change also helps merge code treat
settings values more like black boxes, and interfere less with settings parsing
and retrieval.
2019-11-13 15:23:06 -05:00
Russell Yanofsky
0fa54358b0 refactor: Add ArgsManager::GetSettingsList method
Add for consistency with ArgsManager::GetSetting method and to make setting
types accessible to ArgsManager callers and tests (test added next commit).

This commit does not change behavior.
2019-11-13 04:20:30 -05:00
Russell Yanofsky
3e185522ac refactor: Get rid of ArgsManagerHelper class
Suggested by John Newbery <john@johnnewbery.com>
https://github.com/bitcoin/bitcoin/pull/15934#issuecomment-551969778

This commit does not change behavior.
2019-11-13 04:20:30 -05:00
Russell Yanofsky
dc0f148074 refactor: Replace FlagsOfKnownArg with GetArgFlags
Rename suggested by João Barbosa <joao.paulo.barbosa@gmail.com>
https://github.com/bitcoin/bitcoin/pull/16545#issuecomment-519048000

This also gets rid of ArgsManager::NONE constant, which was an implementation
detail not meant to be used by ArgsManager callers.

Finally this reverts a change from 7f40528cd5
https://github.com/bitcoin/bitcoin/pull/15934 adding "-" characters to argument
names. Better for GetArgFlags to require "-" prefixes for consistency with
other ArgsManager methods, and to be more efficient later when GetArg functions
need to call GetArgFlags (https://github.com/bitcoin/bitcoin/pull/16545)

This commit does not change behavior.
2019-11-13 04:20:30 -05:00
Russell Yanofsky
57e8b7a727 refactor: Clean up includeconf comments
Suggested by Antoine Riard <ariard@student.42.fr>
https://github.com/bitcoin/bitcoin/pull/15934#discussion_r344291875

and John Newbery <john@johnnewbery.com>
https://github.com/bitcoin/bitcoin/pull/15934#discussion_r344271224

This commit does not change behavior.
2019-11-13 04:20:30 -05:00
Russell Yanofsky
3f7dc9b808 refactor: Clean up long lines in settings code
Suggested by James O'Beirne <james.obeirne@gmail.com>
https://github.com/bitcoin/bitcoin/pull/15934#discussion_r344366743

This commit does not change behavior.
2019-11-13 04:20:30 -05:00
Hennadii Stepanov
b131524137
util: Add missing headers to util/fees.cpp 2019-11-12 09:30:52 +02:00
Russell Yanofsky
7f40528cd5 Deduplicate settings merge code
Get rid of settings merging code in util/system.cpp repeated 5 places,
inconsistently:

- ArgsManagerHelper::GetArg
- ArgsManagerHelper::GetNetBoolArg
- ArgsManager::GetArgs
- ArgsManager::IsArgNegated
- ArgsManager::GetUnsuitableSectionOnlyArgs

Having settings merging code separated from parsing simplifies parsing somewhat
(for example negated values can simply be represented as false values instead
of partially cleared or emply placeholder lists).

Having settings merge happen one place instead of 5 makes it easier to add new
settings sources and harder to introduce new inconsistencies in the way
settings are merged.

This commit does not change behavior in any way.
2019-11-07 23:08:22 -04:00
Russell Yanofsky
9dcb952fe5 Add util::Settings struct and helper functions.
Implement merging of settings from different sources (command line and config
file) separately from parsing code in system.cpp, so it is easier to add new
sources.

Document current inconsistent merging behavior without changing it.

This commit only adds new settings code without using it. The next commit calls
the new code to replace existing code in system.cpp.

Co-authored-by: John Newbery <john@johnnewbery.com>
2019-11-07 22:08:22 -05:00
Russell Yanofsky
e2e37cfe8a Remove includeconf nested scope
Easier to review ignoring whitespace

Suggestion from John Newbery <john@johnnewbery.com> in
https://github.com/bitcoin/bitcoin/pull/15934#discussion_r343806860
2019-11-07 22:08:22 -05:00
Russell Yanofsky
5a84aa880f Rename includeconf variables for clarity
includeconf -> conf_file_names
to_include -> conf_file_name
include_config -> conf_file_stream

Suggestion from John Newbery <john@johnnewbery.com> in
https://github.com/bitcoin/bitcoin/pull/15934#discussion_r343905138
2019-11-07 22:04:42 -05:00
Russell Yanofsky
dc8e1e7548 Clarify emptyIncludeConf logic
Suggestion from John Newbery <john@johnnewbery.com> in
https://github.com/bitcoin/bitcoin/pull/15934#discussion_r343795528
2019-11-07 22:01:22 -05:00
Wladimir J. van der Laan
463eab5e14
Merge : doc: Bip70 removal follow-up
3ed8e3d079 doc: Remove explicit network name references (Fabian Jahr)
d6e493f0c2 wallet: Remove left-over BIP70 comment (Fabian Jahr)

Pull request description:

  A small follow-up to  which removed BIP70 support.

  1. Removes one leftover mention of BIP70 in a comment.
  2. Removes BIP70 reference in comments on network/chain name strings. These can be removed as they are not really helpful and also incorrect: BIP70 only defines "main" and "test" but not "regtest". If/When signet gets merged we will add another name to the list that is not defined in BIP70. Mostly there is also an exhaustive list of the options included in the comment anyway.

  If we would like to keep an identifier for this naming scheme, I would suggest switching to something more generic, like 'short chain name'. Happy to implement that if that is preferred. Alternatively, we could add a reference to `CBaseChainParams`. That would also mean we don't have to change these lines again for signet.

ACKs for top commit:
  MarcoFalke:
    ACK 3ed8e3d079

Tree-SHA512: 9a7c0b9cacbb67bd31a089ffdc6f1ebc7f336493e2c8266eb697da34dce2b505a431d5639a3e4fc34f9287361343e861b55dc2662e0a1d2095cc1046db77d6ee
2019-11-02 14:47:41 +01:00
Fabian Jahr
3ed8e3d079 doc: Remove explicit network name references 2019-11-01 12:06:35 +01:00