Skip to content

Commit

Permalink
Merge bitcoin#28890: rpc: Remove deprecated -rpcserialversion
Browse files Browse the repository at this point in the history
fa46cc2 Remove deprecated -rpcserialversion (MarcoFalke)

Pull request description:

  The flag is problematic for many reasons:

  * It is deprecated
  * It is a global flag, requiring a restart to change, as opposed to a flag that can be set on each RPC invocation
  * It may be hidden in config files by accident, hard to debug, causing LND crashes and bugs, see bitcoin#28730 (comment)
  * It makes performance improvements harder to implement: bitcoin#17529 (comment)

  Fix all issues by removing it.

  If there is a use-case, likely a per-RPC flag can be added, if needed.

ACKs for top commit:
  ajtowns:
    crACK fa46cc2
  TheCharlatan:
    lgtm ACK fa46cc2

Tree-SHA512: 96ba1c60356ce93954fe5c2a59045771c6d1516ad0d9dc436ef1800a1f1b0153f0d5fb78ca99d53ad54ba25fbce36962bdf1d4325aceedfc8154a61347a6a915
fanquake committed Jan 5, 2024
2 parents d445545 + fa46cc2 commit 143ace6
Showing 14 changed files with 19 additions and 80 deletions.
4 changes: 2 additions & 2 deletions src/core_io.h
Original file line number Diff line number Diff line change
@@ -51,9 +51,9 @@ bool ParseHashStr(const std::string& strHex, uint256& result);
// core_write.cpp
UniValue ValueFromAmount(const CAmount amount);
std::string FormatScript(const CScript& script);
std::string EncodeHexTx(const CTransaction& tx, const bool without_witness = false);
std::string EncodeHexTx(const CTransaction& tx);
std::string SighashToStr(unsigned char sighash_type);
void ScriptToUniv(const CScript& script, UniValue& out, bool include_hex = true, bool include_address = false, const SigningProvider* provider = nullptr);
void TxToUniv(const CTransaction& tx, const uint256& block_hash, UniValue& entry, bool include_hex = true, bool without_witness = false, const CTxUndo* txundo = nullptr, TxVerbosity verbosity = TxVerbosity::SHOW_DETAILS);
void TxToUniv(const CTransaction& tx, const uint256& block_hash, UniValue& entry, bool include_hex = true, const CTxUndo* txundo = nullptr, TxVerbosity verbosity = TxVerbosity::SHOW_DETAILS);

#endif // BITCOIN_CORE_IO_H
12 changes: 4 additions & 8 deletions src/core_write.cpp
Original file line number Diff line number Diff line change
@@ -140,14 +140,10 @@ std::string ScriptToAsmStr(const CScript& script, const bool fAttemptSighashDeco
return str;
}

std::string EncodeHexTx(const CTransaction& tx, const bool without_witness)
std::string EncodeHexTx(const CTransaction& tx)
{
DataStream ssTx;
if (without_witness) {
ssTx << TX_NO_WITNESS(tx);
} else {
ssTx << TX_WITH_WITNESS(tx);
}
ssTx << TX_WITH_WITNESS(tx);
return HexStr(ssTx);
}

@@ -172,7 +168,7 @@ void ScriptToUniv(const CScript& script, UniValue& out, bool include_hex, bool i
out.pushKV("type", GetTxnOutputType(type));
}

void TxToUniv(const CTransaction& tx, const uint256& block_hash, UniValue& entry, bool include_hex, bool without_witness, const CTxUndo* txundo, TxVerbosity verbosity)
void TxToUniv(const CTransaction& tx, const uint256& block_hash, UniValue& entry, bool include_hex, const CTxUndo* txundo, TxVerbosity verbosity)
{
CHECK_NONFATAL(verbosity >= TxVerbosity::SHOW_DETAILS);

@@ -268,6 +264,6 @@ void TxToUniv(const CTransaction& tx, const uint256& block_hash, UniValue& entry
}

if (include_hex) {
entry.pushKV("hex", EncodeHexTx(tx, without_witness)); // The hex-encoded transaction. Used the name "hex" to be consistent with the verbose output of "getrawtransaction".
entry.pushKV("hex", EncodeHexTx(tx)); // The hex-encoded transaction. Used the name "hex" to be consistent with the verbose output of "getrawtransaction".
}
}
11 changes: 0 additions & 11 deletions src/init.cpp
Original file line number Diff line number Diff line change
@@ -655,7 +655,6 @@ void SetupServerArgs(ArgsManager& argsman)
argsman.AddArg("-rpccookiefile=<loc>", "Location of the auth cookie. Relative paths will be prefixed by a net-specific datadir location. (default: data dir)", ArgsManager::ALLOW_ANY, OptionsCategory::RPC);
argsman.AddArg("-rpcpassword=<pw>", "Password for JSON-RPC connections", ArgsManager::ALLOW_ANY | ArgsManager::SENSITIVE, OptionsCategory::RPC);
argsman.AddArg("-rpcport=<port>", strprintf("Listen for JSON-RPC connections on <port> (default: %u, testnet: %u, signet: %u, regtest: %u)", defaultBaseParams->RPCPort(), testnetBaseParams->RPCPort(), signetBaseParams->RPCPort(), regtestBaseParams->RPCPort()), ArgsManager::ALLOW_ANY | ArgsManager::NETWORK_ONLY, OptionsCategory::RPC);
argsman.AddArg("-rpcserialversion", strprintf("Sets the serialization of raw transaction or block hex returned in non-verbose mode, non-segwit(0) (DEPRECATED) or segwit(1) (default: %d)", DEFAULT_RPC_SERIALIZE_VERSION), ArgsManager::ALLOW_ANY, OptionsCategory::RPC);
argsman.AddArg("-rpcservertimeout=<n>", strprintf("Timeout during HTTP requests (default: %d)", DEFAULT_HTTP_SERVER_TIMEOUT), ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::RPC);
argsman.AddArg("-rpcthreads=<n>", strprintf("Set the number of threads to service RPC calls (default: %d)", DEFAULT_HTTP_THREADS), ArgsManager::ALLOW_ANY, OptionsCategory::RPC);
argsman.AddArg("-rpcuser=<user>", "Username for JSON-RPC connections", ArgsManager::ALLOW_ANY | ArgsManager::SENSITIVE, OptionsCategory::RPC);
@@ -1025,16 +1024,6 @@ bool AppInitParameterInteraction(const ArgsManager& args)
if (args.GetBoolArg("-peerbloomfilters", DEFAULT_PEERBLOOMFILTERS))
nLocalServices = ServiceFlags(nLocalServices | NODE_BLOOM);

if (args.GetIntArg("-rpcserialversion", DEFAULT_RPC_SERIALIZE_VERSION) < 0)
return InitError(Untranslated("rpcserialversion must be non-negative."));

if (args.GetIntArg("-rpcserialversion", DEFAULT_RPC_SERIALIZE_VERSION) > 1)
return InitError(Untranslated("Unknown rpcserialversion requested."));

if (args.GetIntArg("-rpcserialversion", DEFAULT_RPC_SERIALIZE_VERSION) == 0 && !IsDeprecatedRPCEnabled("serialversion")) {
return InitError(Untranslated("-rpcserialversion=0 is deprecated and will be removed in the future. Specify -deprecatedrpc=serialversion to allow anyway."));
}

// Also report errors from parsing before daemonization
{
kernel::Notifications notifications{};
3 changes: 0 additions & 3 deletions src/interfaces/chain.h
Original file line number Diff line number Diff line change
@@ -335,9 +335,6 @@ class Chain
//! Run function after given number of seconds. Cancel any previous calls with same name.
virtual void rpcRunLater(const std::string& name, std::function<void()> fn, int64_t seconds) = 0;

//! Current RPC serialization flags.
virtual bool rpcSerializationWithoutWitness() = 0;

//! Get settings value.
virtual common::SettingsValue getSetting(const std::string& arg) = 0;

1 change: 0 additions & 1 deletion src/node/interfaces.cpp
Original file line number Diff line number Diff line change
@@ -777,7 +777,6 @@ class ChainImpl : public Chain
{
RPCRunLater(name, std::move(fn), seconds);
}
bool rpcSerializationWithoutWitness() override { return RPCSerializationWithoutWitness(); }
common::SettingsValue getSetting(const std::string& name) override
{
return args().GetSetting(name);
8 changes: 4 additions & 4 deletions src/rest.cpp
Original file line number Diff line number Diff line change
@@ -316,7 +316,7 @@ static bool rest_block(const std::any& context,
switch (rf) {
case RESTResponseFormat::BINARY: {
DataStream ssBlock;
ssBlock << RPCTxSerParams(block);
ssBlock << TX_WITH_WITNESS(block);
std::string binaryBlock = ssBlock.str();
req->WriteHeader("Content-Type", "application/octet-stream");
req->WriteReply(HTTP_OK, binaryBlock);
@@ -325,7 +325,7 @@ static bool rest_block(const std::any& context,

case RESTResponseFormat::HEX: {
DataStream ssBlock;
ssBlock << RPCTxSerParams(block);
ssBlock << TX_WITH_WITNESS(block);
std::string strHex = HexStr(ssBlock) + "\n";
req->WriteHeader("Content-Type", "text/plain");
req->WriteReply(HTTP_OK, strHex);
@@ -722,7 +722,7 @@ static bool rest_tx(const std::any& context, HTTPRequest* req, const std::string
switch (rf) {
case RESTResponseFormat::BINARY: {
DataStream ssTx;
ssTx << RPCTxSerParams(tx);
ssTx << TX_WITH_WITNESS(tx);

std::string binaryTx = ssTx.str();
req->WriteHeader("Content-Type", "application/octet-stream");
@@ -732,7 +732,7 @@ static bool rest_tx(const std::any& context, HTTPRequest* req, const std::string

case RESTResponseFormat::HEX: {
DataStream ssTx;
ssTx << RPCTxSerParams(tx);
ssTx << TX_WITH_WITNESS(tx);

std::string strHex = HexStr(ssTx) + "\n";
req->WriteHeader("Content-Type", "text/plain");
7 changes: 3 additions & 4 deletions src/rpc/blockchain.cpp
Original file line number Diff line number Diff line change
@@ -187,7 +187,7 @@ UniValue blockToJSON(BlockManager& blockman, const CBlock& block, const CBlockIn
// coinbase transaction (i.e. i == 0) doesn't have undo data
const CTxUndo* txundo = (have_undo && i > 0) ? &blockUndo.vtxundo.at(i - 1) : nullptr;
UniValue objTx(UniValue::VOBJ);
TxToUniv(*tx, /*block_hash=*/uint256(), /*entry=*/objTx, /*include_hex=*/true, /*without_witness=*/RPCSerializationWithoutWitness(), txundo, verbosity);
TxToUniv(*tx, /*block_hash=*/uint256(), /*entry=*/objTx, /*include_hex=*/true, txundo, verbosity);
txs.push_back(objTx);
}
break;
@@ -736,10 +736,9 @@ static RPCHelpMan getblock()

const CBlock block{GetBlockChecked(chainman.m_blockman, *pblockindex)};

if (verbosity <= 0)
{
if (verbosity <= 0) {
DataStream ssBlock;
ssBlock << RPCTxSerParams(block);
ssBlock << TX_WITH_WITNESS(block);
std::string strHex = HexStr(ssBlock);
return strHex;
}
2 changes: 1 addition & 1 deletion src/rpc/mining.cpp
Original file line number Diff line number Diff line change
@@ -399,7 +399,7 @@ static RPCHelpMan generateblock()
obj.pushKV("hash", block_out->GetHash().GetHex());
if (!process_new_block) {
DataStream block_ser;
block_ser << RPCTxSerParams(*block_out);
block_ser << TX_WITH_WITNESS(*block_out);
obj.pushKV("hex", HexStr(block_ser));
}
return obj;
4 changes: 2 additions & 2 deletions src/rpc/rawtransaction.cpp
Original file line number Diff line number Diff line change
@@ -62,7 +62,7 @@ static void TxToJSON(const CTransaction& tx, const uint256 hashBlock, UniValue&
// Blockchain contextual information (confirmations and blocktime) is not
// available to code in bitcoin-common, so we query them here and push the
// data into the returned UniValue.
TxToUniv(tx, /*block_hash=*/uint256(), entry, /*include_hex=*/true, RPCSerializationWithoutWitness(), txundo, verbosity);
TxToUniv(tx, /*block_hash=*/uint256(), entry, /*include_hex=*/true, txundo, verbosity);

if (!hashBlock.IsNull()) {
LOCK(cs_main);
@@ -383,7 +383,7 @@ static RPCHelpMan getrawtransaction()
}

if (verbosity <= 0) {
return EncodeHexTx(*tx, /*without_witness=*/RPCSerializationWithoutWitness());
return EncodeHexTx(*tx);
}

UniValue result(UniValue::VOBJ);
5 changes: 0 additions & 5 deletions src/rpc/server.cpp
Original file line number Diff line number Diff line change
@@ -597,9 +597,4 @@ void RPCRunLater(const std::string& name, std::function<void()> func, int64_t nS
deadlineTimers.emplace(name, std::unique_ptr<RPCTimerBase>(timerInterface->NewTimer(func, nSeconds*1000)));
}

bool RPCSerializationWithoutWitness()
{
return (gArgs.GetIntArg("-rpcserialversion", DEFAULT_RPC_SERIALIZE_VERSION) == 0);
}

CRPCTable tableRPC;
12 changes: 0 additions & 12 deletions src/rpc/server.h
Original file line number Diff line number Diff line change
@@ -16,8 +16,6 @@

#include <univalue.h>

static const unsigned int DEFAULT_RPC_SERIALIZE_VERSION = 1;

class CRPCCommand;

namespace RPCServer
@@ -183,14 +181,4 @@ void InterruptRPC();
void StopRPC();
std::string JSONRPCExecBatch(const JSONRPCRequest& jreq, const UniValue& vReq);

// Drop witness when serializing for RPC?
bool RPCSerializationWithoutWitness();

template<typename T>
auto RPCTxSerParams(T&& t)
{
if (RPCSerializationWithoutWitness()) return TX_NO_WITNESS(t);
return TX_WITH_WITNESS(t);
}

#endif // BITCOIN_RPC_SERVER_H
3 changes: 1 addition & 2 deletions src/wallet/rpc/transactions.cpp
Original file line number Diff line number Diff line change
@@ -783,8 +783,7 @@ RPCHelpMan gettransaction()
ListTransactions(*pwallet, wtx, 0, false, details, filter, /*filter_label=*/std::nullopt);
entry.pushKV("details", details);

std::string strHex = EncodeHexTx(*wtx.tx, pwallet->chain().rpcSerializationWithoutWitness());
entry.pushKV("hex", strHex);
entry.pushKV("hex", EncodeHexTx(*wtx.tx));

if (verbose) {
UniValue decoded(UniValue::VOBJ);
4 changes: 2 additions & 2 deletions src/zmq/zmqpublishnotifier.cpp
Original file line number Diff line number Diff line change
@@ -250,7 +250,7 @@ bool CZMQPublishRawBlockNotifier::NotifyBlock(const CBlockIndex *pindex)
return false;
}

ss << RPCTxSerParams(block);
ss << TX_WITH_WITNESS(block);

return SendZmqMessage(MSG_RAWBLOCK, &(*ss.begin()), ss.size());
}
@@ -260,7 +260,7 @@ bool CZMQPublishRawTransactionNotifier::NotifyTransaction(const CTransaction &tr
uint256 hash = transaction.GetHash();
LogPrint(BCLog::ZMQ, "Publish rawtx %s to %s\n", hash.GetHex(), this->address);
DataStream ss;
ss << RPCTxSerParams(transaction);
ss << TX_WITH_WITNESS(transaction);
return SendZmqMessage(MSG_RAWTX, &(*ss.begin()), ss.size());
}

23 changes: 0 additions & 23 deletions test/functional/feature_segwit.py
Original file line number Diff line number Diff line change
@@ -88,14 +88,11 @@ def set_test_params(self):
self.extra_args = [
[
"-acceptnonstdtxn=1",
"-rpcserialversion=0",
"-deprecatedrpc=serialversion",
"-testactivationheight=segwit@165",
"-addresstype=legacy",
],
[
"-acceptnonstdtxn=1",
"-rpcserialversion=1",
"-testactivationheight=segwit@165",
"-addresstype=legacy",
],
@@ -224,18 +221,6 @@ def run_test(self):
self.fail_accept(self.nodes[0], "mandatory-script-verify-flag-failed (Witness program hash mismatch)", p2sh_ids[NODE_0][P2WPKH][0], sign=False, redeem_script=witness_script(False, self.pubkey[0]))
self.fail_accept(self.nodes[0], "mandatory-script-verify-flag-failed (Witness program was passed an empty witness)", p2sh_ids[NODE_0][P2WSH][0], sign=False, redeem_script=witness_script(True, self.pubkey[0]))

self.log.info("Verify block and transaction serialization rpcs return differing serializations depending on rpc serialization flag")
assert self.nodes[2].getblock(blockhash, False) != self.nodes[0].getblock(blockhash, False)
assert self.nodes[1].getblock(blockhash, False) == self.nodes[2].getblock(blockhash, False)

for tx_id in segwit_tx_list:
tx = tx_from_hex(self.nodes[2].gettransaction(tx_id)["hex"])
assert self.nodes[2].getrawtransaction(tx_id, False, blockhash) != self.nodes[0].getrawtransaction(tx_id, False, blockhash)
assert self.nodes[1].getrawtransaction(tx_id, False, blockhash) == self.nodes[2].getrawtransaction(tx_id, False, blockhash)
assert self.nodes[0].getrawtransaction(tx_id, False, blockhash) != self.nodes[2].gettransaction(tx_id)["hex"]
assert self.nodes[1].getrawtransaction(tx_id, False, blockhash) == self.nodes[2].gettransaction(tx_id)["hex"]
assert self.nodes[0].getrawtransaction(tx_id, False, blockhash) == tx.serialize_without_witness().hex()

# Coinbase contains the witness commitment nonce, check that RPC shows us
coinbase_txid = self.nodes[2].getblock(blockhash)['tx'][0]
coinbase_tx = self.nodes[2].gettransaction(txid=coinbase_txid, verbose=True)
@@ -276,9 +261,6 @@ def run_test(self):
# tx3 (non-segwit input, paying to a non-segwit output).
# tx1 is allowed to appear in the block, but no others.
txid1 = send_to_witness(1, self.nodes[0], find_spendable_utxo(self.nodes[0], 50), self.pubkey[0], False, Decimal("49.996"))
hex_tx = self.nodes[0].gettransaction(txid)['hex']
tx = tx_from_hex(hex_tx)
assert tx.wit.is_null() # This should not be a segwit input
assert txid1 in self.nodes[0].getrawmempool()

tx1_hex = self.nodes[0].gettransaction(txid1)['hex']
@@ -613,11 +595,6 @@ def run_test(self):
assert_equal(self.nodes[1].gettransaction(txid, True)["txid"], txid)
assert_equal(self.nodes[1].listtransactions("*", 1, 0, True)[0]["txid"], txid)

self.log.info('Test negative and unknown rpcserialversion throw an init error')
self.stop_node(0)
self.nodes[0].assert_start_raises_init_error(["-rpcserialversion=-1"], "Error: rpcserialversion must be non-negative.")
self.nodes[0].assert_start_raises_init_error(["-rpcserialversion=100"], "Error: Unknown rpcserialversion requested.")

def mine_and_test_listunspent(self, script_list, ismine):
utxo = find_spendable_utxo(self.nodes[0], 50)
tx = CTransaction()

0 comments on commit 143ace6

Please sign in to comment.