You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Merge bitcoin/bitcoin#30532: refactor: remove deprecated TxidFromString() in favour of transaction_identifier::FromHex()
f553e6d refactor: remove TxidFromString (stickies-v)
285ab50 test: replace WtxidFromString with Wtxid::FromHex (stickies-v)
9a0b2a6 fuzz: increase FromHex() coverage (stickies-v)
526a87b test: add uint256::FromHex unittest coverage (stickies-v)
Pull request description:
Since bitcoin/bitcoin@fab6ddb, `TxidFromString()` has been deprecated because it is less robust than the `transaction_identifier::FromHex()` introduced in [the same PR](bitcoin/bitcoin#30482). Specifically, it tries to recover from length-mismatches, recover from untrimmed whitespace, 0x-prefix and garbage at the end, instead of simply requiring exactly 64 hex-only characters.
In this PR, `TxidFromString` is removed completely to clean up the code and prevent further unsafe usage. Unit and fuzz test coverage on `uint256::FromHex()` and functions that wrap it is increased.
Note: `TxidFromSring` allowed users to prefix strings with "0x", this is no longer allowed for `transaction_identifier::FromHex()`, so a helper function for input validation may prove helpful in the future _(this overlaps with the `uint256::uint256S()` vs `uint256::FromHex()` future cleanup)_. It is not relevant to this PR, though, besides the fact that this unused (except for in tests) functionality is removed.
The only users of `TxidFromString` are:
- `test`, where it is straightforward to drop in the new `FromHex()` methods without much further concern
- `qt` coincontrol. There is no need for input validation here, but txids are not guaranteed to be 64 characters. This is already handled by the existing code, so again, using `FromHex()` here seems quite straightforward.
Addresses @maflcko's suggestion: bitcoin/bitcoin#30482 (comment)
Also removes `WtxidFromString()`, which is a test-only helper function.
### Testing GUI changes
To test the GUI coincontrol affected lines, `regtest` is probably the easiest way to quickly get some test coins, you can use e.g.
```
alias cli="./src/bitcoin-cli -regtest"
cli createwallet "coincontrol"
# generate 10 spendable outputs on 1 address
cli generatetoaddress 10 $(cli -rpcwallet=coincontrol getnewaddress)
# generate 10 spendable outputs on another address
cli generatetoaddress 10 $(cli -rpcwallet=coincontrol getnewaddress)
# make previous outputs spendable
cli generatetoaddress 100 $(cli -rpcwallet=coincontrol getnewaddress)
```
ACKs for top commit:
maflcko:
re-ACK f553e6d 🔻
hodlinator:
ACK f553e6d
paplorinc:
ACK f553e6d
TheCharlatan:
Nice, ACK f553e6d
Tree-SHA512: c1c7e6ea4cbf05cf660ba178ffc4f35f0328f7aa6ad81872e2462fb91a6a22e4681ff64b3d0202a5a9abcb650c939561585cd309164a69ab6081c0765ee271ef
// disable some items (like Copy Transaction ID, lock, unlock) for tree roots in context menu
177
-
if(item->data(COLUMN_ADDRESS, TxHashRole).toString().length() == 64) // transaction hash is 64 characters (this means it is a child node, so it is not a parent node in tree mode)
178
-
{
177
+
auto txid{Txid::FromHex(item->data(COLUMN_ADDRESS, TxHashRole).toString().toStdString())};
178
+
if (txid) { // a valid txid means this is a child node, and not a parent node in tree mode
if (model->wallet().isLockedCoin(COutPoint(TxidFromString(item->data(COLUMN_ADDRESS, TxHashRole).toString().toStdString()), item->data(COLUMN_ADDRESS, VOutRole).toUInt())))
181
-
{
180
+
if (model->wallet().isLockedCoin(COutPoint(*txid, item->data(COLUMN_ADDRESS, VOutRole).toUInt()))) {
182
181
lockAction->setEnabled(false);
183
182
unlockAction->setEnabled(true);
184
-
}
185
-
else
186
-
{
183
+
} else {
187
184
lockAction->setEnabled(true);
188
185
unlockAction->setEnabled(false);
189
186
}
190
-
}
191
-
else// this means click on parent node in tree mode -> disable all
192
-
{
187
+
} else { // this means click on parent node in tree mode -> disable all
voidCoinControlDialog::viewItemChanged(QTreeWidgetItem* item, int column)
342
337
{
343
-
if (column == COLUMN_CHECKBOX && item->data(COLUMN_ADDRESS, TxHashRole).toString().length() == 64) // transaction hash is 64 characters (this means it is a child node, so it is not a parent node in tree mode)
0 commit comments