This corrects a bug the case of tying group size where the code may
fail to select the group with the newest member. Since newest time
is the final selection criteria, failing to break ties on it
on the step before can undermine the final selection.
Tied netgroups are very common.
With automatic tor HS support in place we should probably not be providing
absolute protection for local peers, since HS inbound could be used to
attack pretty easily. Instead, this counts on the latency metric inside
AttemptToEvictConnection to privilege actually local peers.
The setAskFor duplicate elimination was too eager and removed entries
when we still had no getdata response, allowing the peer to keep
INVing and not responding.
Nagle appears to be a significant contributor to latency now that the static
sleeps are gone. Most of our messages are relatively large compared to
IP + TCP so I do not expect this to create enormous overhead.
This may also reduce traffic burstyness somewhat.
Conflicts:
src/net.cpp
Rebased-From: a4e28b3d1e5c95eb0c87f144851cd65048c3e0bc
Github-Pull: #6867
This adds SCRIPT_VERIFY_LOW_S to STANDARD_SCRIPT_VERIFY_FLAGS which
will make the node require the canonical 'low-s' encoding for
ECDSA signatures when relaying or mining.
Consensus behavior is unchanged.
The rational is explained in a81cd96805:
Absent this kind of test ECDSA is not a strong signature as given
a valid signature {r, s} both that value and {r, -s mod n} are valid.
These two encodings have different hashes allowing third parties a
vector to change users txids. These attacks are avoided by picking
a particular form as canonical and rejecting the other form(s); in
the of the LOW_S rule, the smaller of the two possible S values is
used.
If widely deployed this change would eliminate the last remaining
known vector for nuisance malleability on boring SIGHASH_ALL
p2pkh transactions. On the down-side it will block most
transactions made by sufficiently out of date software.
Unlike the other avenues to change txids on boring transactions this
one was randomly violated by all deployed bitcoin software prior to
its discovery. So, while other malleability vectors where made
non-standard as soon as they were discovered, this one has remained
permitted. Even BIP62 did not propose applying this rule to
old version transactions, but conforming implementations have become
much more common since BIP62 was initially written.
Bitcoin Core has produced compatible signatures since a28fb70e in
September 2013, but this didn't make it into a release until 0.9
in March 2014; Bitcoinj has done so for a similar span of time.
Bitcoinjs and electrum have been more recently updated.
This does not replace the need for BIP62 or similar, as miners can
still cooperate to break transactions. Nor does it replace the
need for wallet software to handle malleability sanely[1]. This
only eliminates the cheap and irritating DOS attack.
[1] On the Malleability of Bitcoin Transactions
Marcin Andrychowicz, Stefan Dziembowski, Daniel Malinowski, Łukasz Mazurek
http://fc15.ifca.ai/preproceedings/bitcoin/paper_9.pdf
Rebased-From: b196b685c9089b74fd4ff3d9a28ea847ab36179b
Github-Pull: #6769
When the internal miner is enabled at the start of a new node, there
is an near instant assert in TestBlockValidity because its attempting
to mine a block before the top checkpoint.
Also avoids a data race around vNodes.
It's reasonable that automatic coin selection will not pick a zero
value txout, but they're actually spendable; and you should know
if you have them. Listing also makes them available to tools like
dust-b-gone.
On hosts that had spent some time with a failed internet connection their
nAttempts penalty was going through the roof (e.g. thousands for all peers)
and as a result the connect search was pegging the CPU and failing to get
more than a 4 connections after days of running (because it was taking so
long per try).
Normally bitcoin core does not display any network originated strings without
sanitizing or hex encoding. This wasn't done for strcommand in many places.
This could be used to play havoc with a terminal displaying the logs,
especially with printtoconsole in use.
Thanks to Evil-Knievel for reporting this issue.
This avoids a regression for issues like #334 where high speed
repeated connections eventually run the HTTP client out of
sockets because all of theirs end up in time_wait.
Maybe the trade-off here is suboptimal, but if both choices will
fail then we prefer fewer changes until the root cause is solved.
It turns out that some miners have been staying with old versions of
Bitcoin Core because their software behaves poorly with persistent
connections and the Bitcoin Core thread and connection limits.
What happens is that underlying HTTP libraries leave connections open
invisibly to their users and then the user runs into the default four
thread limit. This looks like Bitcoin Core is unresponsive to RPC.
There are many things that should be improved in Bitcoin Core's behavior
here, e.g. supporting more concurrent connections, not tying up threads
for idle connections, disconnecting kept-alive connections when limits
are reached, etc. All are fairly big, risky changes.
Disabling keep-alive is a simple workaround. It's often not easy to turn
off the keep-alive support in the client where it may be buried in some
platform library.
If you are one of the few who really needs persistent connections you
probably know that you want them and can find a switch; while if you
don't and the misbehavior is hitting you it is hard to discover the
source of your problems is keepalive related. Given that it is best
to default to off until they're handled better.
This harmonizes the block fetch timeout with the existing ping timeout
and eliminates a guaranteed eventual failure from congestion collapse
for a network operating right at its limit.
It's unlikely that we wouldn't suffer other failures if we were really
anywhere near the network's limit, and a complete avoidance of congestion
collapse risk requires (I think) an exponential back-off. So this isn't
a major concern, but I think it's also useful for reducing the complexity
of understanding out timeouts.
8dccba6 fail immediately on an empty signature (Cory Fields)
dad7764 depends: bump openssl to 1.0.1k (Cory Fields)
488ed32 consensus: guard against openssl's new strict DER checks (Cory Fields)
Previously the minRelayTxFee was only enforced on user specified values.
It was possible for smartfee to produce a fee below minRelayTxFee which
would just result in the transaction getting stuck because it can't be
relayed.
This also introduces a maxtxfee option which sets an absolute maximum
for any fee created by the wallet, with an intention of increasing
user confidence that the automatic fees won't burn them. This was
frequently a concern even before smartfees.
If the configured fee policy won't even allow the wallet to meet the relay
fee the transaction creation may be aborted.
'Sane' was already defined by this code as:
fee.GetFeePerK() > minRelayFee.GetFeePerK() * 10000
But sanity was only enforced for data loaded from disk.
Note that this is a pretty expansive definition of 'sane': A 10 BTC
fee is still passes the test if its on a 100kb transaction.
This prevents a single insane fee on the network from making us reject
our stored fee data at start. We still may reject valid saved fee
state if minRelayFee is changed between executions.
This also reduces the risk and limits the damage from a cascading
failure where one party pays a bunch of insane fees which cases
others to pay insane fees.
TLS is subject to downgrade attacks when SSLv3 is available, and
SSLv3 has vulnerabilities.
The popular solution is to disable SSLv3. On the web this breaks
some tiny number of very old clients. While Bitcoin RPC shouldn't
be exposed to the open Internet, it also shouldn't be exposed to
really old SSL implementations, so it shouldn't be a major issue
for us to disable SSLv3.
There is more information on the downgrade attacks and disabling
SSLv3 at https://disablessl3.com/ .
Previously -proxy was not setting the proxy for IsLimited networks, so
if you set your configuration to be onlynet=tor you wouldn't get an
IPv4 proxy set.
The payment protocol gets its proxy configuration from the IPv4 proxy,
and so it would experience a connection leak.
This addresses issue #5355 and also clears up a cosmetic bug where
getinfo proxy output shows nothing when onlynet=tor is set.
This is a simplified re-do of closed pull #3088.
This patch eliminates the privacy and reliability problematic use
of centralized web services for discovering the node's addresses
for advertisement.
The Bitcoin protocol already allows your peers to tell you what
IP they think you have, but this data isn't trustworthy since
they could lie. So the challenge is using it without creating a
DOS vector.
To accomplish this we adopt an approach similar to the one used
by P2Pool: If we're announcing and don't have a better address
discovered (e.g. via UPNP) or configured we just announce to
each peer the address that peer told us. Since peers could
already replace, forge, or drop our address messages this cannot
create a new vulnerability... but if even one of our peers is
giving us a good address we'll eventually make a useful
advertisement.
We also may randomly use the peer-provided address for the
daily rebroadcast even if we otherwise have a seemingly routable
address, just in case we've been misconfigured (e.g. by UPNP).
To avoid privacy problems, we only do these things if discovery
is enabled.
CCryptoKeyStore::Unlock has a loop to attempt decrypting each key which
only executes once, likely due to a simple mistake when the code was
originally written.
This patch fixes the behavior by making it check all keys. It also adds
a fatal assertion in the case some decrypt but some do not, since that
indicates that the wallet is in some kind of really bad state.
This may make unlocking noticeably slower on wallets with many keys.
This removes some inconsistencies in what worked and didn't work in
safemode. Now only RPCs involved in getting balances or sending
funds are disabled.
Previously you could mine but not submit blocks— but we may need more
blocks to resolve a fork that triggered safe mode in the first place,
and the non-submission was not reliable since some miners submit
blocks via multiple means. There were also a number of random commands
disabled that had nothing to do with the blockchain like verifymessage.
Thanks to earlz for pointing out that there were some moderately cheap
ways to maliciously trigger safe mode, which brought attention to
the fact that safemode wasn't used in a very intelligent way.
When we are over our outbound limit ThreadSocketHandler would try to
keep the connection if the peer was addnoded.
This didn't actually work for two reasons: It didn't actually run
the accept code due to mistaken code flow, and because we have a
limited number of outbound semaphores it couldn't actually use the
connection.
Instead it leaked the socket, which might have caused issue #4034.
This patch just takes out the non-functioning white-listing for now.
This shows the minimum relay fee for non-free transactions in btc/kb.
The armory developers requested this so that they can prevent users from
creating transactions that not even their local bitcoind will relay.
This also slightly reorders the getinfo output so that the fee related
lines are grouped and changes the help text to reflect that the units
are btc/kb.
Commit be066fad accidentally removed the hex field.
This gets in the way of doing offline signing.
(credit belongs to sipa for actually looking for the
cause instead of being lazy like me and just shrugging
and writing the scriptpubkey from the asm.)
There were quite a few places where assert() was used with side effects,
making operation with NDEBUG non-functional. This commit fixes all the
cases I know about, but also adds an #error on NDEBUG because the code
is untested without assertions and may still have vulnerabilities if
used without assert.
Seems like there are a lot of advertised testnet HS nodes that
don't actually work. Lack of the testnet port on the example
HS config might be one reason.
This changes the priority calculation to not include the size of per-txin
data including up to 110 bytes of scriptsig so that transactions which
sweep up extra UTXO don't lose priority relative to ones that don't.
I'd toyed with some other variations, but it seems like any formulation
which results in an incentive stronger than making them not count will
sometimes create incentives to add extra outputs so that you have
extra inputs to consume later. The maximum credit is limited so that
users don't lose the disincentive to stuff random data in their
transactions, the limit of 110 is based on the size of a P2SH
redemption with a compressed public key.
This shouldn't need a staged deployment because the priority is not
used as a relay criteria, only a mining criteria.
As we'd previously learned, OSX's fsync is a data eating lie.
Since 0.8.4 we're still getting some reports of disk corruption on
OSX but now all of it looks like the block files have gotten out of
sync with the database. It turns out that we were still using fsync()
on the block files, so this isn't surprising.
This also makes negative transaction versions non-standard.
This avoids an issue triggered in block 256818 where transactions with
negative version numbers were incorrectly serialized into the UTXO set.
On restart nodes detect the inconsistency and refuse to start so long as
a block with these transactions is inside the self-consistency check
window, logging "coin database inconsistencies found". The software
recommends reindexing, but reindexing does not correct the problem.
This should be fixed by changing the chainstate serialization, but
working around it seems harmless for now because the version is not
used by any network rule currently.
A patch free workaround is to start with -checklevel=2 which skips
the consistency checks, but the IsStandard change is important for
miners in order to protect unpatched nodes.
I've seen users confused multiple times thinking they
should be using -tor to set their tor proxy and then
finding in horror that they were still connecting to
the IPv4 internet.
Even Jeff guesses wrong about what the knob does, so
I think we should rename it. This leaves the old
knob working, we can pull it out completely in a
later release.
There have been several incidents where mainnet experimentation with
raw transactions resulted in insane fees. This is hard to prevent
in the raw transaction api because the inputs may not be known.
Since sending doesn't work if the inputs aren't known, we can catch
it there.
This rejects fees > than 10000 * nMinRelayTxFee or 1 BTC with the
defaults and can be overridden with a bool at the rpc.
We're not seeing large reorgs that would justify waiting a large
amount past the rule required maturity, and the extra three
hours is just a nuisance. Take one more block to at least give
the 100th block time to propagate.
With an encrypted wallet the GUI was prompting for a passphrase every time
the user requested a new address. This is unnecessary, increases the
exposure to keyboard sniffers, and discourages using fresh addresses for
every transaction.
Instead only prompt for a passphrase when the keypool runs out, also call
the new address function with the flag that prevents reuse.
Thanks to AlexNagy on IRC for pointing this out and who wouldn't take any
lip from a curmudgeonly developer and insisted on what he knew to be true.
This reduces a peer's ability to attack network resources by
using a full bloom filter, but without reducing the usability
of bloom filters. It sets a default match everything filter
for peers and it generalizes a prior optimization to
cover more cases.
This fixes test_bitcoin failures on openbsd reported by dhill on IRC.
On some systems rand() is a simple LCG over 2^31 and so it produces
an even-odd sequence. ApproximateBestSubset was only using the least
significant bit and so every run of the iterative solver would be the
same for some inputs, resulting in some pretty dumb decisions.
Using something other than the least significant bit would paper over
the issue but who knows what other way a system's rand() might get us
here. Instead we use an internal RNG with a period of something like
2^60 which is well behaved. This also makes it possible to make the
selection deterministic for the tests, if we wanted to implement that.
If the user was really after the fastest possible confirmation times
they would be manually setting a fee. In cases where the wallet builds
a transaction with a priority that is too low to qualify as free until
the next block, go ahead without a fee. Confirmation frequently takes
multiple blocks even when a minimum fee is provided.
This avoids a potential crash when trying to read the scrippubkeys on
transactions where the first input IsMine but some of the rest are not
when running listaddressgroupings.
CreateNewBlock was reading pindexBest at the start before taking the lock
so it was possible to have the the block content not match the prevheader
and this can also trigger a newly added assert in ConnectBlock.
I noticed this during a code review after twobitcoins reported that ab91bf39
(BIP30 for all blocks) could cause a null dereference on a modified node
that mined during the IBD, or on testnet when it reached heights 91842 and
91880 due to CreateNewBlock calling ConnectBlock with pindex->phashBlock NULL.
Matt pointed out some time ago that there existed a minor DOS
attack where a node in its initial block download could be wedged
by an overwrite attack in a fork created between checkpoints before
a time where BIP30 was enforced. Now that the BIP30 timestamp
is irreversibly past the check can be more aggressive and apply to
all blocks except the two historic violations.
If our IRC nick is in use (because some other node thinks it has
the same address we think we have) don't fruitlessly try to reconnect
using that name forever. After three tries, give up and use a random
nick. Either we'll learn a new local address from IRC and switch
to that, or it was right and the other guy is advertising for us.
This avoids a pessimal case where a second testnet node behind
a nat is unable to get any peers because he can't get on IRC.
Previously Bitcoin would refuse to use IRC if it was either not
accepting inbound connections or not making outbound. Instead this
changes it to not use IRC only if it's not doing either or if
IPv4 is off completely. If Bitcoin is not listening this will use
the default random nicks rather than the IP based ones.
This applies on top of the coincontrol listaddressgroupings patch
and makes finding eligible outputs from the groups returned
by listaddressgroupings possible.
Gregory Maxwell