Use of `sprintf` is seen as a red flag as many of its uses are insecure.
OpenBSD warns about it while compiling, and some modern platforms, e.g.
[cloudlibc from cloudabi](https://github.com/NuxiNL/cloudlibc) don't
even provide it anymore.
Although our uses of these functions are secure, it can't hurt to
replace them anyway. There are only 3 occurences left, all in the
tests.
- Change initializeResult(int) to initializeResult(bool) to avoid
implicit type conversion.
- Use EXIT_FAILURE and EXIT_SUCCESS instead of magic numbers.
- Remove the argument from shutdownResult(int); it was called with a
constant argument.
Warnings introduced by commit e2e2f4c "Return errors from importmulti if
complete rescans are not successful" and reported by Pavel Janík
<Pavel@Janik.cz> in https://github.com/bitcoin/bitcoin/pull/9773 and
https://github.com/bitcoin/bitcoin/pull/9827
wallet/test/wallet_tests.cpp: In member function ‘void wallet_tests::rescan::test_method()’:
wallet/test/wallet_tests.cpp:377:17: warning: declaration of ‘wallet’ shadows a global declaration [-Wshadow]
CWallet wallet;
An effort to reduce the size of AppInitMain().
The removed code upgrades the location of the block files when
upgrading to 0.8. 0.8 seems to be the oldest version still in use.
If the code was compiled with newer (>=3.17) kernel headers but executed
on a system without the system call, every use of random would crash the
program. Add a fallback for that case.
Move the OS random test to a sanity check function that is called every
time bitcoind is initialized.
Keep `src/test/random_tests.cpp` for the case that later random tests
are added, and keep a rudimentary test that just calls the sanity check.
These are available in sandboxes without access to files or
devices. Also [they are safer and more straightforward](https://en.wikipedia.org/wiki/Entropy-supplying_system_calls)
to use than `/dev/urandom` as reading from a file has quite a few edge
cases:
- Linux: `getrandom(buf, buflen, 0)`. [getrandom(2)](http://man7.org/linux/man-pages/man2/getrandom.2.html)
was introduced in version 3.17 of the Linux kernel.
- OpenBSD: `getentropy(buf, buflen)`. The [getentropy(2)](http://man.openbsd.org/cgi-bin/man.cgi/OpenBSD-current/man2/getentropy.2)
function appeared in OpenBSD 5.6.
- FreeBSD and NetBSD: `sysctl(KERN_ARND)`. Not sure when this was added
but it has existed for quite a while.
Alternatives:
- Linux has sysctl `CTL_KERN` / `KERN_RANDOM` / `RANDOM_UUID`
which gives 16 bytes of randomness. This may be available
on older kernels, however [sysctl is deprecated on Linux](https://lwn.net/Articles/605392/)
and even removed in some distros so we shouldn't use it.
Add tests for `GetOSRand()`:
- Test that no error happens (otherwise `RandFailure()` which aborts)
- Test that all 32 bytes are overwritten (initialize with zeros, try multiple times)
Discussion:
- When to use these? Currently they are always used when available.
Another option would be to use them only when `/dev/urandom` is not
available. But this would mean these code paths receive less testing,
and I'm not sure there is any reason to prefer `/dev/urandom`.
Closes: #9676
3ddfe29 netbase: Do not print an error on connection timeouts through proxy (Wladimir J. van der Laan)
13f6085 netbase: Make InterruptibleRecv return an error code instead of bool (Wladimir J. van der Laan)
f6d18f5 Qt/Intro: Explain a bit more what will happen first time (Luke Dashjr)
50c5657 Qt/Intro: Storage shouldn't grow significantly with pruning enabled (Luke Dashjr)
9adb694 Qt/Intro: Move sizeWarningLabel text into C++ code (Luke Dashjr)
* Extends -dns parameter (via fNameLookup) to control these two new
parameters in addition to -addnode, -connect, and -seednode
* Moves fNameLookup assignment earlier as needed
* Changes -proxy and -onion to use Lookup() instead of LookupNumeric()
Remove "nLowestTimestamp <= chainActive.Tip()->GetBlockTimeMax()" check from
importmulti, which is always true because nLowestTimestamp is set to the
minimum of the most recent block time and all the imported key timestamps,
which is necessarily lower than the maximum block time.
e662af3 Use 2 hour grace period for key timestamps in importmulti rescans (Russell Yanofsky)
38d3e9e [qa] Extend import-rescan.py to test imports on pruned nodes. (Russell Yanofsky)
c28583d [qa] Extend import-rescan.py to test specific key timestamps (Russell Yanofsky)
8be0866 [qa] Simplify import-rescan.py (Russell Yanofsky)
- If the -maxsigcachesize parameter is set to zero, setup a minimum sized
sigcache (2 elements) rather than segfaulting.
- Handle maxsigcachesize being negative
- Handle maxsigcachesize being too large
A new AssertLockHeld(cs_wallet) call was added in commit a58370e
"Dedup nTimeFirstKey update logic" (part of PR #9108).
The lock held assertion will fail when loading prexisting wallets files from
before the #9108 merge that have watch-only keys.
Fixes a bug in AcceptBlock() in invoking CheckBlock() with incorrect
arguments, and restores a call to CheckBlock() from ProcessNewBlock()
as belt-and-suspenders.
Updates the (overspecified) tests to match behavior.
Because it is used inconsistently at least version 5.4.0 of g++ to
complains about methods that don't use override. There is two ways to go
about this: remove override from the methods having it, or add it to the
methods missing it. I chose the second.
d943491 qa: add a test to detect leaky p2p messages (Cory Fields)
8650bbb qa: Expose on-connection to mininode listeners (Matt Corallo)
5b5e4f8 qa: mininode learns when a socket connects, not its first action (Matt Corallo)
cbfc5a6 net: require a verack before responding to anything else (Cory Fields)
8502e7a net: parse reject earlier (Cory Fields)
c45b9fb net: correctly ban before the handshake is complete (Cory Fields)
66f861a Add a test for P2P inactivity timeouts (Matt Corallo)
b436f92 qa: Expose on-connection to mininode listeners (Matt Corallo)
8aaba7a qa: mininode learns when a socket connects, not its first action (Matt Corallo)
2cbd119 Disconnect peers which we do not receive VERACKs from within 60 sec (Matt Corallo)
266a811 Use MTP for importmulti "now" timestamps (Russell Yanofsky)
3cf9917 Add test to check new importmulti "now" value (Russell Yanofsky)
442887f Require timestamps for importmulti keys (Russell Yanofsky)
7179e7c qt: Periodic translations update (Wladimir J. van der Laan)
5e903a5 devtools: Handle Qt formatting characters edge-case in update-translations.py (Wladimir J. van der Laan)
7a8c251901 made this logic hard to follow. After that change, messages would
not be sent to a peer via SendMessages() before the handshake was complete, but
messages could still be sent as a response to an incoming message.
For example, if a peer had not yet sent a verack, we wouldn't notify it about
new blocks, but we would respond to a PING with a PONG.
This change makes the behavior straightforward: until we've received a verack,
never send any message other than version/verack/reject.
The behavior until a VERACK is received has always been undefined, this change
just tightens our policy.
This also makes testing much easier, because we can now connect but not send
version/verack, and anything sent to us is an error.
Prior to this change, all messages were ignored until a VERSION message was
received, as well as possibly incurring a ban score.
Since REJECT messages can be sent at any time (including as a response to a bad
VERSION message), make sure to always parse them.
Moving this parsing up keeps it from being caught in the
if (pfrom->nVersion == 0) check below.
7a8c251901 made a change to avoid getting into SendMessages() until the
version handshake (VERSION + VERACK) is complete. That was done to avoid
leaking out messages to nodes who could connect, but never bothered sending
us their version/verack.
Unfortunately, the ban tally and possible disconnect are done as part of
SendMessages(). So after 7a8c251901, if a peer managed to do something
bannable before completing the handshake (say send 100 non-version messages
before their version), they wouldn't actually end up getting
disconnected/banned. That's fixed here by checking the banscore as part of
ProcessMessages() in addition to SendMessages().
a60677e Pre-0.14.0 hardcoded seeds update (Wladimir J. van der Laan)
bfa9393 contrib/seeds: Update PATTERN_AGENT (Wladimir J. van der Laan)
4dfac2c Update seeds tooling to Python 3 (Wladimir J. van der Laan)
When importing a watch-only address over importmulti with a specific timestamp,
the wallet's nTimeFirstKey is currently set to 1. After this change, the
provided timestamp will be used and stored as metadata associated with
watch-only key. This can improve wallet performance because it can avoid the
need to scan the entire blockchain for watch only addresses when timestamps are
provided.
Also adds timestamp to validateaddress return value (needed for tests).
Fixes#9034.
Additionally, accept a "now" timestamp, to allow avoiding rescans for keys
which are known never to have been used.
Note that the behavior when "now" is specified is slightly different than the
previous behavior when no timestamp was specified at all. Previously, when no
timestamp was specified, it would avoid rescanning during the importmulti call,
but set the key's nCreateTime value to 1, which would not prevent future block
reads in later ScanForWalletTransactions calls. With this change, passing a
"now" timestamp will set the key's nCreateTime to the current block time
instead of 1.
Fixes#9491
These are (afaik) all long-standing races or concurrent accesses. Going
forward, we can clean these up so that they're not all individual atomic
accesses.
- Reintroduce cs_vRecv to guard receive-specific vars
- Lock vRecv/vSend for CNodeStats
- Make some vars atomic.
- Only set the connection time in CNode's constructor so that it doesn't change
Minimum boost version was bumped to 1.47.0 in #8920, which
means the configure step won't even pass with older boost.
This version has boost filesystem v3, which means the
(crappy) fallbacks for older versions can go.
If a timeout happens while reading the proxy response, this effectively
means we timed out while connecting to the remote node. This is very
common for Tor, so do not print an error message.
a9baa6d Bugfix: Qt/Intro: Pruned nodes never require *more* space (Luke Dashjr)
93ffba7 Bugfix: Qt/Intro: Chain state needs to be stored even with the full blockchain (Luke Dashjr)
c8cee26 Qt/Intro: Update block chain size (Luke Dashjr)
618ee92 Further-enforce lockordering by enforcing directly after TRY_LOCKs (Matt Corallo)
2a962d4 Fixup style a bit by moving { to the same line as if statements (Matt Corallo)
8465631 Always enforce lock strict lock ordering (try or not) (Matt Corallo)
fd13eca Lock cs_vSend and cs_inventory in a consistent order even in TRY (Matt Corallo)
The initialization order of global data structures in different
implementation units is undefined. Making use of this is essentially
gambling on what the linker does, the so-called [Static initialization
order fiasco](https://isocpp.org/wiki/faq/ctors#static-init-order).
In this case it apparently worked on Linux but failed on OpenBSD and
FreeBSD.
To create it on first use, make the registration structure local to
a function.
Fixes#8910.
6dbfe08 [qa] test signrawtransaction merge with missing inputs (Matt Corallo)
ec4f7e4 [qa] Add second input to signrawtransaction test case (Matt Corallo)
691710a [qa] Test that decoderawtransaction throws with extra data appended (Matt Corallo)
922bea9 Better handle invalid parameters to signrawtransaction (Matt Corallo)
7ea0ad5 Fail in DecodeHexTx if there is extra data at the end (Matt Corallo)
0729102 Net: pass interruptMsgProc as const where possible (Jorge Timón)
fc7f2ff Net: Make CNetMsgMaker more const (Jorge Timón)
d45955f Net: CConnman: Make some methods const (Jorge Timón)
Easier to understand what the button does (it resets the graph view).
'Clear' might mean that the graph is emptied and stops updating, whereas
its easier to see that you're just starting fresh with 'Reset'.
08bb6f4 net: log an error rather than asserting if send version is misused (Cory Fields)
7a8c251 net: Disallow sending messages until the version handshake is complete (Cory Fields)
12752af net: don't run callbacks on nodes that haven't completed the version handshake (Cory Fields)
2046617 net: deserialize the entire version message locally (Cory Fields)
80ff034 Dont deserialize nVersion into CNode, should fix#9212 (Matt Corallo)
Preserve comment, order form, and account strings from the original wallet
transaction. Also set fTimeReceivedIsTxTime and fFromMe fields for consistency
with CWallet::CreateTransaction. The latter two fields don't influence current
wallet behavior, but do record that the transaction originated in the wallet
instead of coming from the network or sendrawtransaction.
This silently skips trying to merge signatures from inputs which
do not exist from transactions provided to signrawtransaction,
instead of hitting an assert.
Since ForEach* are can be used to send messages to all nodes, the caller may
end up sending a message before the version handshake is complete. To limit
this, filter out these nodes. While we're at it, may as well filter out
disconnected nodes as well.
Delete unused methods rather than updating them.
This avoids having some vars set if the version negotiation fails.
Also copy it all into CNode at the same site. nVersion and
fSuccessfullyConnected are set last, as they are the gates for the other vars.
Make them atomic for that reason.
Once the CNode has been added to vNodes, it is possible that it is
disconnected+deleted in the socket handler thread. However, after
that we now call InitializeNode, which accesses the pnode.
helgrind managed to tickle this case (somehow), but I suspect it
requires in immensely braindead scheduler.
More accurate than simply adding one byte per input, and properly handles the
case where the original transaction happened to have very small signatures
2366180 Do not add to vNodes until fOneShot/fFeeler/fAddNode have been set (Matt Corallo)
3c37dc4 Ensure cs_vNodes is held when using the return value from FindNode (Matt Corallo)
5be0190 Delete some unused (and broken) functions in CConnman (Matt Corallo)
4b189c1 Change bumpfee result value from 'oldfee' to 'origfee'. (Alex Morcos)
0c0c63f Introduce WALLET_INCREMENTAL_RELAY_FEE (Alex Morcos)
e8021ec Use CWallet::GetMinimumFee in bumpfee (Alex Morcos)
ae9719a Refactor GetMinimumFee to give option of providing targetFee (Alex Morcos)
fe8e8ef [rpc] Add incremental relay fee to getnetworkinfo (Alex Morcos)
6b331e6 Fix to have miner test aware of new separate block min tx fee (Alex Morcos)
de6400d Fix missing use of dustRelayFee (Alex Morcos)
5b15870 Use incrementalRelayFee for BIP 125 replacement (Alex Morcos)
The result value indicates the actual fee on the transaction that was replaced. But there is an error message which uses the description 'oldfee' to refer to the original fee rate applied to the new transaction's estimated max size. It was confusing that two different uses of 'oldfee' had two different numeric values.
Have wallet's default bump value be higher than the default incrementalRelayFee to future proof against changes to incremental relay fee. Only applies when not setting the fee rate directly.
Use the wallet's fee calculation logic to properly clamp fee against minimums and maximums when calculating the fee for a bumpfee transaction. Unless totalFee is explictly given, in which case, manually check against min, but do nothing to adjust given fee.
In all cases do a final check against maxTxFee (after adding any incremental amount).
Before this commit, the checkbox would always start off unchecked. After this
commit it will respect the -walletrbf setting (which is currently false by
default).
The use of mocktime in test logic means that comparisons between
GetTime() and GetTimeMicros()/1000000 are unreliable since the former
can use mocktime values while the latter always gets the system clock;
this changes the networking code's inactivity checks to consistently
use the system clock for inactivity comparisons.
Also remove some hacks from setmocktime() that are no longer needed,
now that we're using the system clock for nLastSend and nLastRecv.
