Change Wallet API get_transaction to use wallet_id parameter. Add test.

[aqk]

Purpose:

WalletRpcApi.get_transaction() would never look at the wallet_id parameter, therefore returned Transactions were never filtered by wallet_id.

Breaking Change! See below.

Current Behavior:

Would always return requested transaction, regardless of requested wallet.

New Behavior:

Will only return the transaction if it belongs to the
Fails if an invalid wallet_id parameter is supplied, or if wallet_id is missing

Security Notes

checking the wallet_id does not confer any security benefits. Anyone with access to the wallet API likely has access to the wallet db.

Design Notes

Personally, I would remove the wallet_id parameter entirely. Its only use currently is a sanity check, and that check could be built into the RPC receiver. All TransactionRecords contain their wallet_id.

We need to get rid of the local wallet_id int for wallet identity purposes. One of the many issues this causes us is that wallet "IDs" can change after a wallet rebuild or chain reorg.

Code notes

I'm guessing get_transaction was introduced as a quick to implement alternative to extending get_transactions

If that is correct, and it is not used in many places, we might consider deprecating get_transaction, especially if all the current use is in-house.

We might want to consider folding this into get_transactions. I'll link the PR for that.

Testing Notes:

[aqk]

By the way, I don't mind if we want to do away with the wallet_id parameter entirely, as it is not currently giving us any security (we would have to test API requests against the "currently logged in" key to be able to reject requests for other wallets than the current one.

As long as the expected API behaviour is specified.

[aqk]

This is the actual change being tested.

https://github.com/Chia-Network/chia-blockchain/pull/17594/files#diff-1bce0e44731d38aa058d7caa1a389bcbe6cd53dd05a025ec787700160f2f5ed0R147

[Quexington]

Is there an issue or something this was fixing? I don't see the wallet parameter being used prior to this change so this is technically breaking backwards compatibility.

[felixbrucker]

I rely on this api and i do not send the wallet_id in the request as that was never required, as such this would be a breaking change indeed. I can't imagine i'm the only one using it.

[emlowe]

I think what wallet_rpc_client does in adding the wallet_id to the JSON is what is wrong and we should just change that - that is, go in the other direction from this PR. So it's a simple one-line change in wallet_rpc_client::get_transaction to fix up that call.

[aqk]

See #17598

[aqk]

#17598

[aqk]

I rely on this api and i do not send the wallet_id in the request as that was never required, as such this would be a breaking change indeed. I can't imagine i'm the only one using it.

Thanks for your input. Ironically, I chose this implementation because I thought it was the most conservative for people who were using the wallet_id param, and thought that it was required.

I prefer the implementation in #17598

[aqk]

I think what wallet_rpc_client does in adding the wallet_id to the JSON is what is wrong and we should just change that - that is, go in the other direction from this PR. So it's a simple one-line change in wallet_rpc_client::get_transaction to fix up that call.

Thanks. That's what I wanted to do originally, but was trying to reduce surprises introduced during the fix.