1e46d8a Get rid of ambiguous OutputType::NONE value (Russell Yanofsky)
Pull request description:
Based on suggestion by @sipa https://github.com/bitcoin/bitcoin/pull/12119#issuecomment-357982763
After #12119, the NONE output type was overloaded to refer to either an output type that couldn't be parsed, or to an automatic change output mode. This change drops the NONE enum and uses a simple bool to indicate parse failure, and a new CHANGE_AUTO enum to refer the change output type.
This change is almost a pure refactoring except it makes RPCs reject empty string ("") address types instead of treating them like they were unset. This simplifies the parsing code a little bit and could prevent RPC usage mistakes. It's noted in the release notes.
Follows up #12408 by @MarcoFalke
Followups for future PRs:
- [ ] Add explicit support for specifying "auto" in `ParseOutputType` as suggested by promag and sipa: https://github.com/bitcoin/bitcoin/pull/12729#issuecomment-374799567 and https://github.com/bitcoin/bitcoin/pull/12729#discussion_r175969481
- [ ] Add wallet `AddressChangeType` method to complement `TransactionChangeType`: https://github.com/bitcoin/bitcoin/pull/12729#discussion_r175969618.
Tree-SHA512: 8b08b272bcb177a0a9e556dcd965840a7fe601ef83ca97938b879c9b1a33b5b3f96939e1bceef11ba7c644ac21bfd6c1dbc6ca715cd1da4ace50475240e4ee48
7d0f80b Use anonymous namespace instead of static functions (Pieter Wuille)
b61fb71 Mention removal of bare multisig IsMine in release notes (Pieter Wuille)
9c2a8b8 Do not treat bare multisig as IsMine (Pieter Wuille)
08f3228 Optimization: only test for witness scripts at top level (Pieter Wuille)
3619735 Track difference between scriptPubKey and P2SH execution in IsMine (Pieter Wuille)
ac6ec62 Switch to a private version of SigVersion inside IsMine (Pieter Wuille)
19fc973 Do not expose SigVersion argument to IsMine (Pieter Wuille)
fb1dfbb Remove unused IsMine overload (Pieter Wuille)
952d821 Make CScript -> CScriptID conversion explicit (Pieter Wuille)
Pull request description:
Currently our wallet code will treat bare multisig outputs (meaning scriptPubKeys with multiple public keys + `OP_CHECKMULTISIG` operator in it) as ours without the user asking for it, as long as all private keys in it are in our wallet.
This is a pointless feature. As it only works when all private keys are in one place, it's useless compared to single key outputs (P2PK, P2PKH, P2WPKH, P2SH-P2WPKH), and worse in terms of space, cost, UTXO size, and ability to test (due to lack of address format for them).
Furthermore, they are problematic in that producing a list of all `scriptPubKeys` we accept is not tractable (it involves all combinations of all public keys that are ours). In further wallet changes I'd like to move to a model where all scriptPubKeys that are treated as ours are explicit, rather than defined by whatever keys we have. The current behavior of the wallet is very hard to model in such a design, so I'd like to get rid of it.
I think there are two options:
* Remove it entirely (do not ever accept bare multisig outputs as ours, unless watched)
* Only accept bare multisig outputs in situations where the P2SH version of that output would also be acceptable
This PR implements the first option. The second option was explored in #12874.
Tree-SHA512: 917ed45b3cac864cee53e27f9a3e900390c576277fbd6751b1250becea04d692b3b426fa09065a3399931013bd579c4f3dbeeb29d51d19ed0c64da75d430ad9a
Based on suggestion by Pieter Wuille <pieter.wuille@gmail.com> at
https://github.com/bitcoin/bitcoin/pull/12119#issuecomment-357982763
After #12119, the NONE output type was overloaded to refer to either an output
type that couldn't be parsed, or to an automatic change output mode. This
change drops the NONE enum and uses a simple bool indicate parse failure, and a
new CHANGE_AUTO enum to refer the change output type.
This change is almost a pure refactoring except it makes RPCs reject empty
string ("") address types instead of treating them like they were unset. This
simplifies the parsing code a little bit and could prevent RPC usage mistakes.
It's noted in the release notes.
4757c04 [config] Remove blockmaxsize option (John Newbery)
Pull request description:
The blockmaxsize option was marked as deprecated in V0.15.1, and code
was added to convert provided blockmaxsize into blockmaxweight. However,
this code was incorrectly implemented, and blockmaxsize was silently
ignored.
No users have complained about blockmaxsize being ignored, so just
remove it in V0.17.
Fixes#12640
cc @ajtowns
Tree-SHA512: 968d71d37bf175c5a02539ddec289a12586f886e1dfe64c1d9aa5e39db48d06d21665153824fac3b11503a55f0812d2f1115a2d726aafd37b76ed629ec0aa671
The blockmaxsize option was marked as deprecated in V0.15.1, and code
was added to convert provided blockmaxsize into blockmaxweight. However,
this code was incorrectly implemented, and blockmaxsize was silently
ignored.
No users have complained about blockmaxsize being ignored, so just
remove it in V0.17.
This change only updates strings and adds RPC aliases, but should simplify the
implementation of address labels in
https://github.com/bitcoin/bitcoin/pull/7729, by getting renaming out of the
way and letting it focus on semantics.
The difference between accounts and labels is that labels apply only to
addresses, while accounts apply to both addresses and transactions
(transactions have "from" and "to" accounts). The code associating accounts
with transactions is clumsy and unreliable so we would like get rid of it.
fac70134a rpc: Update createrawtransaction examples (MarcoFalke)
fa06dfce0 [rpc] createrawtransaction: Accept sorted outputs (MarcoFalke)
8acd25d85 rpc: Allow typeAny in RPCTypeCheck (MarcoFalke)
Pull request description:
The second parameter of the `createrawtransaction` is a dictionary of the outputs. This comes with at least two drawbacks:
* In case of duplicate keys, either of them might silently disappear, with no user feedback at all. A user needs to make other mistakes, but this could eventually lead to abnormal tx fees.
* A dictionary does not guarantee that keys are sorted. Again, a user needs to keep this in mind, as it could eventually lead to excessive tx fees.
Even though my scenario of loss-of-funds is unlikely to happen, I see it as a inconvenience that should be fixed.
Tree-SHA512: cd562f34f7f9f79c7d3433805971325c388c2035611be283980f4049066a622df4f0afdc11d7ac96662260ec0115147cb65e1ab5268f5a1b063242f3fe425f77
* Z is the zone designator for the zero UTC offset.
* T is the delimiter used to separate date and time.
This makes it clear for the end-user that the date/time logged is
specified in UTC and not in the local time zone.
This change should make it easier for users to make complete backups of wallets
because they can now just back up the specified `-wallet=<path>` path directly,
instead of having to back up the specified path as well as the transaction log
directory (for incompletely flushed wallets).
Another advantage of this change is that if two wallets are located in the same
directory, they will now use their own BerkeleyDB environments instead using a
shared environment. Using a shared environment makes it difficult to manage and
back up wallets separately because transaction log files will contain a mix of
data from all wallets in the environment.
Also clean out release notes.
Tree-SHA512: c4d5b52c089e14438be37381e1b0dab3711cc72aa8d345d1024169fff0055f3d021c8ca9d46fb794110694ebcbf7cbca0a12619f650873c9d381530adea7100e
Also warn if bitcoind is configured to use a relative -datadir path.
Specifying paths relative to the current working directory in a daemon process
can be dangerous, because files can fail to be located even if the
configuration doesn't change, but the daemon is started up differently.
Specifying a relative -datadir now adds a warning to the debug log. It would
not be backwards-compatible to forbid relative -datadir paths entirely, and it
could also be also inconvenient for command line testing.
Specifying a relative -walletdir now results in a startup error. But since the
-walletdir option is new in 0.16.0, there should be no compatibility issues.
Another reason not to use working directory paths for -walletdir specifically
is that the default -walletdir is a "wallets" subdirectory inside the datadir,
so it could be surprising that setting -walletdir manually would choose a
directory rooted in a completely different location.
97c3cad [docs] Clarify -walletdir usage (John Newbery)
Pull request description:
After discussion with @ryanofsky around #11687 , I think this documentation is a bit clearer for how the new `-walletdir` argument works.
Tree-SHA512: f279cab82524dbc0d75e6f9891f0e228ec4c8d0df3e16f351057fa243ddd263ff786f05383fd00a09b89edcc07dab211be5b64387f77271edf8af0177bcf667d
GUI wallet uses RBF by default, regardless of -walletrbf.
RPC and debug console in the GUI remain unchanged; they don't
use RBF by default, unless launched with -walletrbf=1.
656fde5 Add script birthtime metadata to dump and import wallet (MeshCollider)
1bab9b2 Add script dump note to RPC help text and release notes (MeshCollider)
68c1e00 Add test for importwallet (MeshCollider)
9e1184d Add dumpwallet scripts test (MeshCollider)
ef0c730 Add scripts to importwallet RPC (MeshCollider)
b702ae8 Add CScripts to dumpwallet RPC (MeshCollider)
cdc260a Add GetCScripts to CBasicKeyStore (MeshCollider)
Pull request description:
As discussed in https://github.com/bitcoin/bitcoin/pull/11289#issuecomment-334600457, adds the CScripts from the wallet to the `dumpwallet` RPC and then allows them to be imported with the `importwallet` RPC. Includes a basic test, and modifies the helptext of the dumpwallet RPC.
Notes:
- Reviewers: use `?w=1` to avoid the indentation-only change in commit `Add scripts to importwallet RPC `
- currently the scripts are followed with `# addr=` comments just as the other keys are, unsure if this might confuse users into thinking all the scripts are for valid P2SH addresses though, but I don't think that should be an issue.
- there are no birthtimes for scripts, so script imports don't affect rescans
- `importwallet` imports the CScripts but I'm not sure how to approach specifying whether scripts are for P2SH addresses, BIP173 addresses, etc. whether that matters or not. Otherwise the RPC helptext might just need modification.
Fixes#11715
Tree-SHA512: 36c55837b3a58b9d3499d4c0c2ae82153d62aa71919e751574651b63a1d2b8ecc83796db4553cc65dad9b5341c3a42ae2fcf4d62598c30af267f8e1461ba8272
aac6b3f067 Update files.md for new wallets/ subdirectory (MeshCollider)
b67342906c Cleanups for walletdir PR (MeshCollider)
Pull request description:
This addresses the remaining nits from https://github.com/bitcoin/bitcoin/pull/11466
- Updates `doc/files.md` with respect to the new default wallet directory
- Fixes @promag and @laanwj's error message nit, and Jonas' release notes nit
- ~Addresses @laanwj's net-specific wallet subdirectory concern in the case that a walletdir is specified~
- Changes the #includes from "" to <> style after #11651
Tree-SHA512: b86bf5fdc4de54c1b0f65b60a83af3cf82b35d216ce9c0de724803bfba6934796238b6c412659dcc29ae2e3e856d4eb97ae777c80f36f4089d8acecfddefe9aa
Change suggested by Cory Fields <cory-nospam-@coryfields.com> who noticed
listsinceblock would ignore invalid block hashes causing it to return a
completely unfiltered list of transactions.
Prevent arbitrary files from being overwritten. There have been reports
that users have overwritten wallet files this way. It may also avoid
other security issues.
Fixes#9934. Adds mention to release notes and adds a test.
Now that 0.15 branch has been split off, master is 0.15.99 (pre-0.16).
Also clean out release notes.
Tree-SHA512: 160f712fae7bfc51e49e36a27ab01f5c243b79a19a70312df95f9bc5cb8067c70aa88911f741fd1625bee549a70655eaea24d98f6049d98c3c14ee1b3143f4cb
This makes the following changes:
* In undo data and the chainstate database, the transaction nVersion
field is removed from the data structures, always written as 0, and
ignored when reading.
* The definition of hash_serialized in gettxoutsetinfo is changed to no
longer incude the nVersion field. It is renamed to hash_serialized_2
to avoid confusion. The new definition also includes transaction
height and coinbase information, as this information was missing
before.
This depends on having a CHashVerifier-based undo data checksum
verifier.
Apart from changing the definition of serialized_hash, downgrading
after using this patch is supported, as no release ever used the value
of nVersion field in UTXO entries.
Wladimir J. van der Laan
