txscript: backport tokenizer from dcrd #1684
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
I... wish I saw this when I was working on #1685 |
|
@cfromknecht Thanks for your PR! We recently merged #1615, which introduced some merge conflicts. Can you please resolve them and fix the edge cases around multisig parsing, so we're closer to having this merged? |
|
Generally speaking, we are super happy for decred backports! As the person who did most of the initial backporting of btcd stuff into decred (times really have changed now that backports got he other way :) ), I know how much of a pain it can be but it is super worth it to get those improvements. |
f038748 to
e4f8c4d
Compare
|
@onyb @jcvernaleo i've rebased on master to remove the merge conflicts. i'm currently going through each commit to check for compilation and resolve the remaining issues to make it ready for review
@kcalvinalvin Haha yeah that's totally my fault for not pushing this up sooner and letting it sit on my fork... I have ~5 or so other optimization branches I should push up so they can at least be tracked |
cfromknecht
added a commit
to cfromknecht/btcd
that referenced
this pull request
Feb 2, 2021
TestHashCacheAddContainsHashes flakes fairly regularly when rebasing PR btcsuite#1684 with: txid <txid> wasn't inserted into cache but was found. With probabilty 1/10^2 there will be no inputs on the transaction. This reduces the entropy in the txid, and I belive is the primary cause of the flake.
e4f8c4d to
8db2852
Compare
|
@onyb @jcvernaleo I went back and touched up all the commits so that each compiles and passes the unit test suite, here's the output: In the process I kept hitting a flake in In the meantime I'll start going through the commits again myself and double checking everything. |
|
Also not noted here, but I already did ~10 or so sync tests on mainnet using this branch back in 2019. @Roasbeef and I will probably run a few more and gather some final benchmarks. |
Pull Request Test Coverage Report for Build 553429087
💛 - Coveralls |
8db2852 to
827c3d7
Compare
|
I went through each commit and reran the before and after benchmarks where applicable, as well as adding a total comparison in the final commit message showing the difference between when the benchmark was added and the final commit. I updated the PR body with the current benchmarks |
This was referenced Feb 3, 2021
Yep, I'm switching over one of my mainnet nodes to run this patch now. |
|
Not a blocker, but #1304 is touching the |
Review progress tracker▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓ 100 %
Note: Using this mostly for my own reference, but also gives a good idea to others how the review is progressing. |
|
Thanks @onyb! I’m currently working on restructuring some of the commits, e.g. some of the witness related refactoring can be moved into more logical spots. So some of the commits in the latter half might get split/moved up. I think reviewers will find it easier to digest that way. Pretty close to finishing that as well as adding some more benchmarks. If you give me until tomorrow I can get it to a place where (I hope) is more stable for review. The contents of the commits shouldn’t change to much though if you want to familiarize yourself with the diffs :) |
|
@cfromknecht No worries, you can go ahead with the refactoring. It seems there won't be too many logical changes to do anyway. 👍 I'm looking forward to your benchmarks. Something like the graphs in decred/dcrd#1656 would be pretty cool. I am currently syncing testnet3 with this branch. Will report back the IBD time once finished. |
|
I'm seeing a ~20% speedup in IBD on testnet3. Benchmark
System specifications
|
This implements an efficient and zero-allocation script tokenizer that is exported to both provide a new capability to tokenize scripts to external consumers of the API as well as to serve as a base for refactoring the existing highly inefficient internal code. It is important to note that this tokenizer is intended to be used in consensus critical code in the future, so it must exactly follow the existing semantics. The current script parsing mechanism used throughout the txscript module is to fully tokenize the scripts into an array of internal parsed opcodes which are then examined and passed around in order to implement virtually everything related to scripts. While that approach does simplify the analysis of certain scripts and thus provide some nice properties in that regard, it is both extremely inefficient in many cases, and makes it impossible for external consumers of the API to implement any form of custom script analysis without manually implementing a bunch of error prone tokenizing code or, alternatively, the script engine exposing internal structures. For example, as shown by profiling the total memory allocations of an initial sync, the existing script parsing code allocates a total of around 295.12GB, which equates to around 50% of all allocations performed. The zero-alloc tokenizer this introduces will allow that to be reduced to virtually zero. The following is a before and after comparison of tokenizing a large script with a high opcode count using the existing code versus the tokenizer this introduces for both speed and memory allocations: benchmark old ns/op new ns/op delta BenchmarkScriptParsing-8 63464 677 -98.93% benchmark old allocs new allocs delta BenchmarkScriptParsing-8 1 0 -100.00% benchmark old bytes new bytes delta BenchmarkScriptParsing-8 311299 0 -100.00% The following is an overview of the changes: - Introduce new error code ErrUnsupportedScriptVersion - Implement zero-allocation script tokenizer - Add a full suite of tests to ensure the tokenizer works as intended and follows the required consensus semantics - Add an example of using the new tokenizer to count the number of opcodes in a script - Update README.md to include the new example - Update script parsing benchmark to use the new tokenizer
This converts the DisasmString function to make use of the new zero-allocation script tokenizer instead of the far less efficient parseScript thereby significantly optimizing the function. In order to facilitate this, the opcode disassembly functionality is split into a separate function called disasmOpcode that accepts the opcode struct and data independently as opposed to requiring a parsed opcode. The new function also accepts a pointer to a string builder so the disassembly can be more efficiently be built. While here, the comment is modified to explicitly call out the script version semantics. The following is a before and after comparison of a large script: benchmark old ns/op new ns/op delta BenchmarkDisasmString-8 102902 40124 -61.01% benchmark old allocs new allocs delta BenchmarkDisasmString-8 46 51 +10.87% benchmark old bytes new bytes delta BenchmarkDisasmString-8 389324 130552 -66.47%
Also remove tests associated with the func accordingly.
This converts the executeOpcode function defined on the engine to accept an opcode and data slice instead of a parsed opcode as a step towards removing the parsed opcode struct and associated supporting code altogether. It also updates all callers accordingly.
827c3d7 to
3b37866
Compare
This converts the callback function defined on the internal opcode struct to accept the opcode and data slice instead of a parsed opcode as the final step towards removing the parsed opcode struct and associated supporting code altogether. It also updates all of the callbacks and tests accordingly and finally removes the now unused parsedOpcode struct. The final results for the raw script analysis and tokenizer optimizations are as follows: benchmark old ns/op new ns/op delta BenchmarkIsPayToScriptHash-8 62393 0.51 -100.00% BenchmarkIsPubKeyHashScript-8 62228 0.56 -100.00% BenchmarkGetSigOpCount-8 61051 658 -98.92% BenchmarkExtractPkScriptAddrsLarge-8 60713 17.2 -99.97% BenchmarkExtractPkScriptAddrs-8 289 17.9 -93.81% BenchmarkIsWitnessPubKeyHash-8 61688 0.42 -100.00% BenchmarkIsUnspendable-8 656 520 -20.73% BenchmarkExtractAtomicSwapDataPushesLarge-8 61332 44.0 -99.93% BenchmarkExtractAtomicSwapDataPushes-8 990 260 -73.74% BenchmarkDisasmString-8 102902 39754 -61.37% BenchmarkGetPreciseSigOpCount-8 130223 715 -99.45% BenchmarkScriptParsing-8 63464 681 -98.93% BenchmarkIsMultisigScriptLarge-8 64166 5.83 -99.99% BenchmarkIsMultisigScript-8 630 58.5 -90.71% BenchmarkPushedData-8 64837 1779 -97.26% BenchmarkCalcSigHash-8 3627895 3605459 -0.62% BenchmarkIsPubKeyScript-8 62323 2.83 -100.00% BenchmarkIsPushOnlyScript-8 62412 569 -99.09% BenchmarkIsWitnessScriptHash-8 61243 0.56 -100.00% BenchmarkGetScriptClass-8 61515 16.4 -99.97% BenchmarkIsNullDataScript-8 62495 2.53 -100.00% BenchmarkIsMultisigSigScriptLarge-8 69328 2.52 -100.00% BenchmarkIsMultisigSigScript-8 2375 141 -94.06% BenchmarkGetWitnessSigOpCountP2WKH-8 504 72.0 -85.71% BenchmarkGetWitnessSigOpCountNested-8 1158 136 -88.26% BenchmarkIsWitnessPubKeyHash-8 68927 0.53 -100.00% BenchmarkIsWitnessScriptHash-8 62774 0.63 -100.00% benchmark old allocs new allocs delta BenchmarkIsPayToScriptHash-8 1 0 -100.00% BenchmarkIsPubKeyHashScript-8 1 0 -100.00% BenchmarkGetSigOpCount-8 1 0 -100.00% BenchmarkExtractPkScriptAddrsLarge-8 1 0 -100.00% BenchmarkExtractPkScriptAddrs-8 1 0 -100.00% BenchmarkIsWitnessPubKeyHash-8 1 0 -100.00% BenchmarkIsUnspendable-8 1 0 -100.00% BenchmarkExtractAtomicSwapDataPushesLarge-8 1 0 -100.00% BenchmarkExtractAtomicSwapDataPushes-8 2 1 -50.00% BenchmarkDisasmString-8 46 51 +10.87% BenchmarkGetPreciseSigOpCount-8 3 0 -100.00% BenchmarkScriptParsing-8 1 0 -100.00% BenchmarkIsMultisigScriptLarge-8 1 0 -100.00% BenchmarkIsMultisigScript-8 1 0 -100.00% BenchmarkPushedData-8 7 6 -14.29% BenchmarkCalcSigHash-8 1335 712 -46.67% BenchmarkIsPubKeyScript-8 1 0 -100.00% BenchmarkIsPushOnlyScript-8 1 0 -100.00% BenchmarkIsWitnessScriptHash-8 1 0 -100.00% BenchmarkGetScriptClass-8 1 0 -100.00% BenchmarkIsNullDataScript-8 1 0 -100.00% BenchmarkIsMultisigSigScriptLarge-8 5 0 -100.00% BenchmarkIsMultisigSigScript-8 3 0 -100.00% BenchmarkGetWitnessSigOpCountP2WKH-8 2 0 -100.00% BenchmarkGetWitnessSigOpCountNested-8 4 0 -100.00% BenchmarkIsWitnessPubKeyHash-8 1 0 -100.00% BenchmarkIsWitnessScriptHash-8 1 0 -100.00% benchmark old bytes new bytes delta BenchmarkIsPayToScriptHash-8 311299 0 -100.00% BenchmarkIsPubKeyHashScript-8 311299 0 -100.00% BenchmarkGetSigOpCount-8 311299 0 -100.00% BenchmarkExtractPkScriptAddrsLarge-8 311299 0 -100.00% BenchmarkExtractPkScriptAddrs-8 768 0 -100.00% BenchmarkIsWitnessPubKeyHash-8 311299 0 -100.00% BenchmarkIsUnspendable-8 1 0 -100.00% BenchmarkExtractAtomicSwapDataPushesLarge-8 311299 0 -100.00% BenchmarkExtractAtomicSwapDataPushes-8 3168 96 -96.97% BenchmarkDisasmString-8 389324 130552 -66.47% BenchmarkGetPreciseSigOpCount-8 623367 0 -100.00% BenchmarkScriptParsing-8 311299 0 -100.00% BenchmarkIsMultisigScriptLarge-8 311299 0 -100.00% BenchmarkIsMultisigScript-8 2304 0 -100.00% BenchmarkPushedData-8 312816 1520 -99.51% BenchmarkCalcSigHash-8 1373812 1290507 -6.06% BenchmarkIsPubKeyScript-8 311299 0 -100.00% BenchmarkIsPushOnlyScript-8 311299 0 -100.00% BenchmarkIsWitnessScriptHash-8 311299 0 -100.00% BenchmarkGetScriptClass-8 311299 0 -100.00% BenchmarkIsNullDataScript-8 311299 0 -100.00% BenchmarkIsMultisigSigScriptLarge-8 330035 0 -100.00% BenchmarkIsMultisigSigScript-8 9472 0 -100.00% BenchmarkGetWitnessSigOpCountP2WKH-8 1408 0 -100.00% BenchmarkGetWitnessSigOpCountNested-8 3200 0 -100.00% BenchmarkIsWitnessPubKeyHash-8 311299 0 -100.00% BenchmarkIsWitnessScriptHash-8 311299 0 -100.00%
3b37866 to
398da74
Compare
|
@onyb new version is up, the commits should be more coherent/uniform now. I also added a few benchmarks for: Log of all commits passing the test suite: https://paste.ubuntu.com/p/qdYwDHpb3p/ |
onyb
reviewed
Feb 6, 2021
| pops, _ := parseScript(scriptPubKey) | ||
|
|
||
| // Treat non P2SH transactions as normal. | ||
| if !(bip16 && isScriptHash(pops)) { |
There was a problem hiding this comment.
Needs an explanation somewhere why bip16 is not used anymore.
There was a problem hiding this comment.
There were no prior instances where GetPreciseSigOpCount was called with the final argument being false. The new diff now checks isScriptHashScript directly. If it's p2sh, then we'll also examine the sigScript for the final push which contains the script itself.
| } | ||
| return script, nil | ||
| return tokenizer.Err() == nil |
There was a problem hiding this comment.
Should we consider an empty script as push-only?
There was a problem hiding this comment.
Yes, this retains the existing behavior in isPushOnly. You can see similar behavior enforced in bitcoind here: https://github.com/bitcoin/bitcoin/blob/4a540683ec40393d6369da1a9e02e45614db936d/src/script/script.cpp#L250
There was a problem hiding this comment.
This is usually used to enforce that the sigScript for a p2sh or nested p2wsh only contains push data op codes. If a sigScript attempts to spend one of these outputs with a blank script, then execution should fail in both scenarios, but it wouldn't explicitly violate the push only requirement.
| if len(script) == 35 && | ||
| script[34] == OP_CHECKSIG && | ||
| script[0] == OP_DATA_33 && | ||
| (script[1] == 0x02 || script[1] == 0x03) { |
There was a problem hiding this comment.
It's great that the new version of the parser is a lot stricter than the one in isPubkey. Not for this PR, but we should think about unit-tests to exercise all these conditions.
| // versions. | ||
| // | ||
| // The error is DEPRECATED and will be removed in the major version bump. | ||
| func IsMultisigScript(script []byte) (bool, error) { |
There was a problem hiding this comment.
Can you add a test for the "zero pubkeys" case? You can add it to the existing benchmark test, or create a new unit test function.
|
@cfromknecht IBD benchmark with #1426:
|
|
@onyb great comments, I pushed fixups for most of them. The ones left are:
I think perhaps this is a bug, but I will need to go through the call sites to be sure.
I believe we do in this case, this also matches the behavior in the prior
Certainly, I'd be happy to do so!
Sure thing I will have that up in the next set of fixups! |
Roasbeef
reviewed
Feb 27, 2021
txscript/script.go
Outdated
| const scriptVersion = 0 | ||
| tokenizer := MakeScriptTokenizer(scriptVersion, sigScript) | ||
| var prevOffset int32 | ||
| for tokenizer.Next() { |
There was a problem hiding this comment.
Note to self to verify this change independently on my end.
| } | ||
| return script, nil | ||
| return tokenizer.Err() == nil |
There was a problem hiding this comment.
Yes, this retains the existing behavior in isPushOnly. You can see similar behavior enforced in bitcoind here: https://github.com/bitcoin/bitcoin/blob/4a540683ec40393d6369da1a9e02e45614db936d/src/script/script.cpp#L250
| } | ||
| return script, nil | ||
| return tokenizer.Err() == nil |
There was a problem hiding this comment.
This is usually used to enforce that the sigScript for a p2sh or nested p2wsh only contains push data op codes. If a sigScript attempts to spend one of these outputs with a blank script, then execution should fail in both scenarios, but it wouldn't explicitly violate the push only requirement.
| pops, _ := parseScript(scriptPubKey) | ||
|
|
||
| // Treat non P2SH transactions as normal. | ||
| if !(bip16 && isScriptHash(pops)) { |
There was a problem hiding this comment.
There were no prior instances where GetPreciseSigOpCount was called with the final argument being false. The new diff now checks isScriptHashScript directly. If it's p2sh, then we'll also examine the sigScript for the final push which contains the script itself.
|
Kicking off a mainnet sync with this branch later today. |
onyb
approved these changes
Apr 10, 2021
There was a problem hiding this comment.
Finally had the chance to come back to this PR and finish reviewing it. I've added some last comments, but apart from that, it looks ready to go in! Going to pre-approve this PR, since the outstanding review comments are minor. 🚀
@Roasbeef There are two commits that'd particularly benefit from a careful pass by you since it's consensus critical code:
As previously mentioned, I've already tested this PR against testnet3 and noticed a 20% speedup with no stability issues. I'm going to do the same for mainnet, but it might take me a while to obtain the results. Should be fine; I got 5G. 😎
Comment on lines
+919
to
+942
| // Check for pay-to-pubkey script. | ||
| if data := extractPubKey(pkScript); data != nil { | ||
| var addrs []btcutil.Address | ||
| addr, err := btcutil.NewAddressPubKey(data, chainParams) | ||
| if err == nil { | ||
| addrs = append(addrs, addr) | ||
| } | ||
| return PubKeyTy, addrs, 1, nil | ||
| } | ||
|
|
||
| case PubKeyTy: | ||
| // A pay-to-pubkey script is of the form: | ||
| // <pubkey> OP_CHECKSIG | ||
| // Therefore the pubkey is the first item on the stack. | ||
| // Skip the pubkey if it's invalid for some reason. | ||
| requiredSigs = 1 | ||
| addr, err := btcutil.NewAddressPubKey(pops[0].data, chainParams) | ||
| if err == nil { | ||
| addrs = append(addrs, addr) | ||
| // Check for multi-signature script. | ||
| const scriptVersion = 0 | ||
| details := extractMultisigScriptDetails(scriptVersion, pkScript, true) | ||
| if details.valid { | ||
| // Convert the public keys while skipping any that are invalid. | ||
| addrs := make([]btcutil.Address, 0, len(details.pubKeys)) | ||
| for _, pubkey := range details.pubKeys { | ||
| addr, err := btcutil.NewAddressPubKey(pubkey, chainParams) | ||
| if err == nil { | ||
| addrs = append(addrs, addr) | ||
| } | ||
| } | ||
| return MultiSigTy, addrs, details.requiredSigs, nil | ||
| } |
There was a problem hiding this comment.
Wondering if it makes sense to have a function like pubKeyToAddrs, just like pubKeyHashToAddrs, and scriptHashToAddrs. It could also be reused for the multi-sig part.
Comment on lines
+931
to
+932
| details := extractMultisigScriptDetails(scriptVersion, pkScript, true) | ||
| if details.valid { |
There was a problem hiding this comment.
Nit: could be a one-line:
Suggested change
| details := extractMultisigScriptDetails(scriptVersion, pkScript, true) | |
| if details.valid { | |
| if details := extractMultisigScriptDetails(scriptVersion, pkScript, true); details.valid { |
| return nil | ||
| } | ||
|
|
||
| // Ensure all executed data push opcodes use the minimal encoding when | ||
| // the minimal data verification flag is set. | ||
| if vm.dstack.verifyMinimalData && vm.isBranchExecuting() && | ||
| pop.opcode.value >= 0 && pop.opcode.value <= OP_PUSHDATA4 { | ||
| op.value >= 0 && op.value <= OP_PUSHDATA4 { |
There was a problem hiding this comment.
Two remarks:
- Perhaps it's better to use
op.value >= OP_0instead ofop.value >= 0. - Isn't
op.valuealways>= 0, by virtue of being abyte?
| // covering 1 through 16 pubkeys, which means this will count any | ||
| // more than that value (e.g. 17, 18 19) as the maximum number of | ||
| // allowed pubkeys. This is, unfortunately, now part of | ||
| // the Bitcion consensus rules, due to historical |
There was a problem hiding this comment.
typo
Suggested change
| // the Bitcion consensus rules, due to historical | |
| // the Bitcoin consensus rules, due to historical |
|
@cfromknecht Just checking if you got the chance to finish up the last few things in this PR. Is there anything I can help you with? |
marni
pushed a commit
to decfi/btcd
that referenced
this pull request
Oct 5, 2021
* Improve error message about non-active segwit on simnet I started playing with simnet and was confronted with error message: ``` [ERR] FNDG: Unable to broadcast funding tx for ChannelPoint(<point>:0): -22: TX rejected: transaction <tx> has witness data, but segwit isn't active yet ``` I wasn't aware of the activation period so I got quite puzzled. Google helped. But I think the message could mention likely cause. Newly it optionally prints something like: ``` (The threshold for segwit activation is 300 blocks on simnet, current best height is 113) ``` * btcctl: add regtest mode to btcctl * build: replace travis-ci with github actions. test go 1.14 use golangci-lint * build: update deps * build: clean linter warnings * btcjson: change getblock default verbosity to 1 This change makes btcd's getblock command match bitcoind's. Previously the default verbosity was 0, which caused errors when using the rpcclient library to connect to a bitcoind node - getblock would unmarshall incorrectly since it didn't expect a verbosity=1 result when it did not specify verbosity. * rpcclient: send legacy GetBlock request for backwards compatibility Without this, users of this library wouldn't be able to issue GetBlock requests to nodes which haven't updated to support the latest request format, namely the use of a single `int` parameter to denote verbosity instead of two `bool`s. * rpcclient: Add cookie auth Based on Hugo Landau's cookie auth implementation for Namecoin's ncdns. Fixes btcsuite#1054 * rpcclient: Refactor cookie caching * rpcclient: Try user+pass auth before cookie auth * rpcclient: Read first line of cookie instead of trimming space * rpcclient: serialize nil inputs to empty list * Improve chain state init efficiency Remove unnecessary slice of all block indexes and remove DB iteration over all block indexes that used to determined the size of the slice. * Add blockchain.NewUtxoEntry() to directly create entries for UtxoViewpoint The current methods to add to a UtxoViewpoint don't allow for a situation where we have only UTXO data but not a whole transaction. This commit allows contstruction of a UtxoEntry without requiring a full MsgTx. AddTxOut() and AddTxOuts() both require a whole transaction, including the inputs, which are only used in order to calculate the txid. In some situations, such as with use of the utreexo accumulator, we only have the utxo data but not the transaction which created it. For reference, utreexo's initial usage of the blockchain.NewUtxoEntry() function is at https://github.com/mit-dci/utreexo/pull/135/files#diff-3f7b8f9991ea957f1f4ad9f5a95415f0R96 * Add getchaintxstats JSON-RPC client command * Add fundrawtransaction RPC call * Add getbalances RPC client command * rpcclient: Add GetTransactionWatchOnly method * peer: knownInventory, sentNonces - use generic lru While here, also rename and generalize limitMap and apply to other maps which need to be bounded. * btcec: Avoid panic in fieldVal.SetByteSlice for large inputs The implementation has been adapted from the dcrec module in dcrd. The bug was initially fixed in decred/dcrd@3d9cda1 while transitioning to a constant time algorithm. A large set of test vectors were subsequently added in decred/dcrd@8c6b52d. The function signature has been preserved for backwards compatibility. This means that returning whether the value has overflowed, and the corresponding test vectors have not been backported. This fixes btcsuite#1170 and closes a previous attempt to fix the bug in btcsuite#1178. * config+service_windows: add flag to disable win service To run integration tests with btcd on Windows in non-interactive environments (such as the Travis build with Windows machines), we need to make sure we can still spawn a child process instead of only a windows background service. * updated docs for getblock-verbosity fixes * Update json_rpc_api.md Corrections suggested by @onyb btcsuite#1608 (comment) * netsync: handle notfound messages from peers backport from decred/dcrd#2253 When a peer sends a notfound message, remove the hash from requested map. Also increase notfound ban score and return early if it disconnects the peer. * release: update release script path * release: remove old scripts and update process doc - remove prep_release.sh and notes.sample - update license in release.sh - add notes for maintainers on the release process - mention CHANGES file modifications * Update CHANGES file for 0.21.0 release Also updated changes for 0.20.1, and added a small note about changes since 0.12.0. * btcd: bump version to v0.21.0-beta * blockchain: remove unknown block version warning * Add rpclient implementation of getdescriptorinfo RPC * peer: prevent last block height going backwards This modifies the UpdateLastBlockHeight function to ensure the new height is after the existing height before updating it in order to prevent it from going backwards so it properly matches the intent of the function which is to report the latest known block height for the peer. Without this change, the value will properly start out at the latest known block height reported by the peer during version negotiation, however, it will be set to lower values when syncing from the peer due to requesting old blocks and blindly updating the height. It also adds a test to ensure proper functionality. This is a backport of decred/dcrd#1747 * Fix monetary unit * rpcserver: add parity with bitcoind for validateaddress Updated the rpcserver handler for validateaddress JSON-RPC command to have parity with the bitcoind 0.20.0 interface. The new fields included are - isscript, iswitness, witness_version, and witness_program. The scriptPubKey field has been left out since it requires wallet access. This update has no impact on the rpcclient.ValidateAddress method, which uses the btcjson.ValidateAddressWalletResult type for modelling the response from bitcoind. * Add getblockfilter JSON-RPC client command Add type for second getblockfilter param * Implement signmessagewithprivkey JSON-RPC command Reuse the Bitcoin message signature header const also in verifymessage. * rpcclient: Implement importmulti JSON-RPC client command * Add Dockerfile to build and run btcd on Docker. * btcd: fix conversion of int to string failing in Go 1.15 * btcjson,wire: fix invalid use of string(x) to convert byte value * Major rework on documentation to make it compatible to readthedocs.org * Added symlink to index.md for github readme preview. * btcd+netsync: support witness tx and block in notfound msg * btcec: set curve name in CurveParams Set curve name(secp256k1) in KoblitzCurve.CurveParams Fixes btcsuite#1564 * btcec: add a comment indicating where curve name taken from Related with btcsuite#1565 * rpcclient: support listtransactions RPC with watchonly argument Co-authored-by: Gert-Jaap Glasbergen <gertjaap@decoscrypto.com> * blockchain: Remove unnecessary tx hash * btcjson: update ListTransactionsResult for Bitcoin 0.20.0 This only adds new fields as optional, in order to make this change backwards compatible with older versions of Bitcoin Core. * chaincfg: Add RegisterHDKeyID func to populate HD key ID pairs Currently, the only way to register HD version bytes is by initializing chaincfg.Params struct, and registering it during package init. RegisterHDKeyID provides a way to populate custom HD version bytes, without having to create new chaincfg.Params instances. This is useful for library packages who want to use non-standard version bytes for serializing extended keys, such as the ones documented in SLIP-0132. This function is complementary to HDPrivateKeyToPublicKeyID, which is used to lookup previously registered key IDs. * Nullable optional JSON-RPC parameters Fix command marshalling dropping params following params with nil value. btcsuite#1591 Allow specifying null parameter value from command line. * GitHub Actions: Enable Go Race detector and code coverage This modifies the goclean.sh script to run tests with the race detector enabled. It also enables code coverage, and uploads the results to coveralls.io. Running tests with -race and -cover flags was disabled in 6487ba1 and 6788df7 respectively, due to some limits on time/goroutines being hit on Travis CI. Since we have migrated to GitHub Actions, it is desirable to bring them back. * rpc: Add getnodeaddresses JSON-RPC support Add NodeAddresses function to rpcserverConnManager interface for fetching known node addresses. * btcjson,rpcclient: add support for PSBT commands to rpcclient * Added ListSinceBlockMinConfWatchOnly method. * wire: add proper types for flag field and improve docs Summary of changes: - Add a new const TxFlagMarker to indicate the flag prefix byte. - Add a new TxFlag type to enumerate the flags supported by the tx parser. This allows us to avoid hardcoded magics, and will make it easier to support new flags in future. - Improve code comments. Closes btcsuite#1598. * removed unnecessary GOMAXPROCS function calls * rpcclient: add deriveaddresses RPC command * ci: add go 1.15 to tests * sample-btcd.conf: fix typo * btcjson: add test for null params in searchrawtransactions Closes PR btcsuite#1476. * GetBlockTemplate RPC client implementation (btcsuite#1629) * GetBlockTemplate RPC client implementation * Txid added to the getblocktemplate result * Omitempty for TxID and improved comment for GetBlockTemplate 'rules' field * rpcclient: implement getaddressinfo command Fields such as label, and labelspurpose are not included, since they are deprecated, and will be removed in Bitcoin Core 0.21. * Fix link to using bootstrap.dat * rpcclient: implement getwalletinfo command * rpcserver: add txid to getblocktemplate response * rpc: add signrawtransactionwithwallet interface Adds interface for issuing a signrawtransactionwithwallet command. Note that this does not add functionality for the btcd rpc server itself, it simply assumes that the RPC client has this ability and gives an API for interacting with the RPC client. rpc: add signrawtransactionwithwallet interface * rpcclient: implement gettxoutsetinfo command * Unmarshal hashes/second as float in GetMiningInfoResult * rpcclient: add more wallet commands Implement backupwallet, dumpwallet, loadwallet and unloadwallet. * btcjson: add new JSON-RPC errors and document them * rpcclient: implement createwallet with functional options * rpcclient: fix documentation typos * integration: allow setting custom btcd exe path To allow using a custom btcd executable, we allow specifying a path to a file. If the path is empty, the harness will fall back to compiling one from scratch. * integration: allow overwriting address generator * integration: allow specifying connection behavior * integration/rpctest: randomizes port in rpctest.New to reduce collisions * btcjson+rpcserverhelp: restore bitcoind compatibility The PR btcsuite#1594 introduced a change that made the order of parameters relevant, if one of them is nil. This makes it harder to be backward compatible with the same JSON message if an existing parameter in bitcoind was re-purposed to have a different meaning. * simplify s[:] to s where s is a slice Found using https://go-critic.github.io/overview#unslice-ref * rpcclient: add ExtraHeaders in ConnConfig * Add support for receiving sendaddrv2 message from a peer * fixed broken link * Add support for arm32v7 in Dockerfile * Fixes btcsuite#1653 * btcjson: Update fields in GetBlockChainInfoResult Update the fields of GetBlockChainInfoResult to reflect the current state of the RPC returned by other full-node implementations. * InitialBlockDownload - Node is in Initial Block Download mode if True. * SizeOnDisk - The estimated size of the block and undo files on disk. * txscript: add benchmark for IsUnspendable - create benchmarks to measure allocations - add test for benchmark input - create a low alloc parseScriptTemplate - refactor parsing logic for a single opcode * txscript/hashcache_test: always add inputs during getTxn TestHashCacheAddContainsHashes flakes fairly regularly when rebasing PR btcsuite#1684 with: txid <txid> wasn't inserted into cache but was found. With probabilty 1/10^2 there will be no inputs on the transaction. This reduces the entropy in the txid, and I belive is the primary cause of the flake. * txscript/hashcache_test: call rand.Seed once in init This resolves the more fundamental flake in the unit tests noted in the prior commit. Because multiple unit tests call rand.Seed in parallel, it's possible they can be executed with the same unix timestamp (in seconds). If the second call happens between generating the hash cache and checking that the cache doesn't contain a random txn, the random transaction is in fact a duplicate of one generated earlier since the RNG state was reset. To remedy, we initialize rand.Seed once in the init function. * btcec: validate R and S signature components in RecoverCompact * Add Batch JSON-RPC support (rpc client & server) * Fix error message returned by EstimateFee When you provide an argument to EstimateFee(numblocks uint32) that exceeds the estimateFeeDepth (which is set to 25), you get an error message that says "can only estimate fees for up to 100 blocks from now". The variable used in the if condition and the variable used for creating the error message should be the same. * docs: update shields * rpcserver: Fix Error message returned by processRequest When processRequest can't find a rpc command, standardCmdResult returns a `btcjson.ErrRPCMethodNotFound` but it gets ignored and a `btcjson.ErrRPCInvalidRequest` is returned instead. This makes processRequest return the right error message. * peer: allow external testing of peer.Peer The previous use of allowSelfConns prevented this, as users aren't able to invoke peer.TstAllowSelfConns themselves due to being part of a test file, which aren't exported at the library level, leading to a "disconnecting peer connected to self" error upon establishing a mock connection between two peers. By including the option at the config level instead (false by default, prevents connections to self) we enable users of the peer library to properly test the behavior of the peer.Peer struct externally. * addrmgr: Use RLock/RUnlock when possible * build: update btcutil dependency * rpcclient: fix documentation typo * btcjson: Updated TxRawResult.Version from int32 to uint32 * wire+chaincfg: add signet params This commit adds all necessary chain parameters for connecting to the public signet network. Reference: bitcoin/bitcoin#18267 * config+params: add signet config option This commit adds the --signet command line flag (or signet config option) for starting btcd in signet mode. * rpcserver: add taproot deployment to getblockchaininfo * btcctl: add signet param This commit adds the --signet command line flag to the btcctl utility. * mining: extract witness commitment add into method * rpctest: add witness commitment when calling CreateBlock If we tried to include transactions having witnesses, the block would be invalid since the witness commitment was not added. * Don't reference the readme that we don't produce * chaincfg: fix deployment bit numbers On signet all previous soft forks and also taproot are always activated, meaning the version is always 0x20000000 for all blocks. To make sure they activate properly in `btcd` we therefore need to use the correct bit to mask the version. This means that on any custom signet there would need to be 2016 blocks mined before SegWit or Taproot can be used. * mempool: add additional test case for inherited RBF replacement In this commit, we add an additional test case for inherited RBF replacement. This test case asserts that if a parent is marked as being replaceable, but the child isn't, then the child can still be replaced as according to BIP 125 it shoudl _inhreit_ the replaceability of its parent. The addition of this test case was prompted by the recently discovered Bitcoin Core "CVE" [1]. It turns out that bitcoind doesn't properly implement BIP 125. Namely it fails to allow a child to "inherit" replaceability if its parent is also replaceable. Our implementation makes this trait rather explicit due to its recursive implementation. Kudos to the original implementer @wpaulino for getting this correct. [1]: https://lists.linuxfoundation.org/pipermail/bitcoin-dev/2021-May/018893.html. * Update CHANGES file for 0.22.0 release * btcd: bump version to v0.22.0-beta * Update release date for v0.22.0-beta in CHANGES file * remove duplicate command Co-authored-by: Antonin Hildebrand <antonin@hildebrand.cz> Co-authored-by: Dan Cline <kidscline01@gmail.com> Co-authored-by: David Hill <dhill@mindcry.org> Co-authored-by: Henry <henry.wfisher@gmail.com> Co-authored-by: Wilmer Paulino <wilmer.paulino@gmail.com> Co-authored-by: Olaoluwa Osuntokun <laolu32@gmail.com> Co-authored-by: JeremyRand <jeremyrand@airmail.cc> Co-authored-by: Torkel Rogstad <torkel@rogstad.io> Co-authored-by: Mikael Lindlof <mikuz.dev@gmail.com> Co-authored-by: adiabat <rx@awsomnet.org> Co-authored-by: Federico Bond <federicobond@gmail.com> Co-authored-by: Anirudha Bose <anirudha.bose@ledger.fr> Co-authored-by: Javed Khan <tuxcanfly@gmail.com> Co-authored-by: Anirudha Bose <anirudha.bose@alumni.cern> Co-authored-by: Oliver Gugger <gugger@gmail.com> Co-authored-by: qqjettkgjzhxmwj <37233887+JettScythe@users.noreply.github.com> Co-authored-by: Dan Cline <dan@dancline.net> Co-authored-by: John C. Vernaleo <jcv@netpurgatory.com> Co-authored-by: wakiyamap <wakiyamap@gmail.com> Co-authored-by: Christian Lehmann <info@legacycode.org> Co-authored-by: yyforyongyu <yy2452@columbia.edu> Co-authored-by: Hanjun Kim <hallazzang@gmail.com> Co-authored-by: Gert-Jaap Glasbergen <gertjaap@decoscrypto.com> Co-authored-by: Calvin Kim <calvin@kcalvinalvin.info> Co-authored-by: Andrew Tugarinov <nalcheg@gmail.com> Co-authored-by: ipriver <g1ran1q@gmail.com> Co-authored-by: Jake Sylvestre <jakesyl@gmail.com> Co-authored-by: Tristyn <tristynstimpson@gmail.com> Co-authored-by: Elliott Minns <elliott.minns@me.com> Co-authored-by: Friedger Müffke <friedger@gmail.com> Co-authored-by: David Mazary <dmaz@vt.edu> Co-authored-by: Armando Ochoa <armando.o.j@gmail.com> Co-authored-by: Liran Sharir <lsharir@paxos.com> Co-authored-by: Iskander Sharipov <quasilyte@gmail.com> Co-authored-by: 10gic <2391796+10gic@users.noreply.github.com> Co-authored-by: Yaacov Akiba Slama <ya@slamail.org> Co-authored-by: ebiiim <mail@ebiiim.com> Co-authored-by: Victor Lavaud <victor.lavaud@gmail.com> Co-authored-by: Vinayak Borkar <vinayakb@gmail.com> Co-authored-by: Steven Kreuzer <skreuzer@FreeBSD.org> Co-authored-by: Conner Fromknecht <conner@lightning.engineering> Co-authored-by: Appelberg-s <Appelberg-s@users.noreply.github.com> Co-authored-by: Gustavo Chain <gchain@pm.me> Co-authored-by: Aurèle Oulès <aurele@oules.com> Co-authored-by: Johan T. Halseth <johanth@gmail.com>
This was referenced Nov 4, 2021
|
Replaced by #1769 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Dusting off this old branch... IIRC it should be mostly complete, there were a few edge cases around multisig parsing that I wanted to revisit. Happy to finish it up if people are interested in it.This is a backport of decred/dcrd#1656 by @davecgh which converts
all script handling to use a combination of raw script analysis and a zero-allocation script tokenizer.
This vastly reduces the amount of memory and number of allocations performed during IBD, see
the original PR for the performance gains realized in dcrd. As a result, dcrd saw a 15% improvement
in IBD due to the reduction in CPU cycles spent performing GC.
TODO:
Before and after benchmarks for various methods after converting to the
combination of raw script analysis + script tokenizer:
Depends on #1689