After hard fork, replay protected tx's require the amount field to be non-zero for previous outputs in signed tx's. JSON RPC code was accepting transactions with this field missing when it should have been throwing an error. Users had the impression the transaction that was signed was good, but when trying to sent to network, they would get an error that the tx is bad. See github issue https://github.com/Bitcoin-ABC/bitcoin-abc/issues/63.
Details
Details
- Reviewers
dgenr8 freetrader deadalnix - Group Reviewers
Restricted Project
make check
also: rpc-tests.py raw_transactions
Diff Detail
Diff Detail
- Repository
- rABC Bitcoin ABC
- Branch
- issue63_amount
- Lint
Lint Passed - Unit
No Test Coverage - Build Status
Buildable 691 Build 691: arc lint + arc unit
Event Timeline
Comment Actions
I need more review and comment since I'm new to this API, honestly. Please advise (see inline comments). Thanks in advance.
| src/rpc/rawtransaction.cpp | ||
|---|---|---|
| 756 ↗ | (On Diff #1101) | My understanding is that the API is to allow for decimal values eg: 2.002 -- I guess we'll call that a 'numeric' in the API even though later down in the function it's basically treated as a string? |
| 945 ↗ | (On Diff #1101) | Can the value be 0? Because the code before, it was 0, and that apparently produced a bad tx. And it can't be negative (can it?) |
Comment Actions
Bump. I need more guidance/discussion here from @deadalnix and/or others. Thanks in advance..
| src/rpc/rawtransaction.cpp | ||
|---|---|---|
| 756 ↗ | (On Diff #1101) | It is a json value of type numeric. |
| 945 ↗ | (On Diff #1101) | Why do you care the value is ? Maybe you want to check for MoneyRange if you feel like it, but checking for zero specifically makes no sense. If you are checking the value, you need to do it properly. |
Comment Actions
@dgenr8 : From your github comment:
Yes, though I've only run it in test. It was previously failing on broadcast, because verification expects the actual amount (not 0) to be signed.
Should I check that the value not be zero in the C++ rpc server side, in addition to existing checks (1. that amount exists and 2. that it is a MoneyRange()) ?
Please advise, thanks.
| src/rpc/rawtransaction.cpp | ||
|---|---|---|
| 912 | It's not about being valid json. It a string. We expect a (floating point) number. | |
Comment Actions
I think you screwed up something while updating the diff. Also please run autopep8 in another diff.
Comment Actions
autopep8 was automatically run for me by arcanist on all python files I touched.
What did I screw up? I don't understand ...