T3225
This diff fixes bytecount related logic and unit tests, including contextual comments.
Details
Details
- Reviewers
Fabien bytesofman - Group Reviewers
Restricted Project - Commits
- rABC79fbb4329daf: [ecash-coinselect] Fix byte count calculation logic
npm test
Diff Detail
Diff Detail
- Repository
- rABC Bitcoin ABC
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
There are a very large number of changes, so older changes are hidden. Show Older Changes
| modules/ecash-coinselect/src/utxo.js | ||
|---|---|---|
| 164 ↗ | (On Diff #41904) | imo the fact that this is p2pkh only should be reflected in the function name |
| 180 ↗ | (On Diff #41904) | |
| 183 ↗ | (On Diff #41904) | |
| 184 ↗ | (On Diff #41904) | padding ? It's signedness |
| 185 ↗ | (On Diff #41904) | |
| 186 ↗ | (On Diff #41904) | same |
| 191 ↗ | (On Diff #41904) | you should not use the average value, but since low S is enforced as a standardness rule, it's fair to use 72 bytes as an upper limit (sighash included). |
| 193 ↗ | (On Diff #41904) | That's wrong. DER signature can be < 71 bytes due to r and S encoding: It's just unlikely; also that doesn't change the conclusion. |
| 201 ↗ | (On Diff #41904) | OK (didn't check) but this has nothing to do in this function that computes the size of a p2pkh only tx |
| 204 ↗ | (On Diff #41904) | Would be nice to have the option to choose but this might be added later. |
| 229 ↗ | (On Diff #41904) | That's wrong. |
| modules/ecash-coinselect/test/utxo.test.js | ||
| 52 ↗ | (On Diff #41904) | The way you computed it, this sentence is wrong. |
| 70 ↗ | (On Diff #41904) | Dito |
| 88 ↗ | (On Diff #41904) | dito |
Comment Actions
Responding to feedback
| modules/ecash-coinselect/src/utxo.js | ||
|---|---|---|
| 164 ↗ | (On Diff #41904) | Updated function name |
| 180 ↗ | (On Diff #41904) | Updated comment |
| 183 ↗ | (On Diff #41904) | Updated comment |
| 184 ↗ | (On Diff #41904) | Updated comment |
| 186 ↗ | (On Diff #41904) | Updated comment |
| 191 ↗ | (On Diff #41904) | Updated comment |
| 193 ↗ | (On Diff #41904) | Updated comment based on minimum size of DER signatures being 64 bytes from the article above, which would make the minimum scriptSig 100 bytes for a minimum p2pkh input byte size of 140 bytes. |
| 201 ↗ | (On Diff #41904) | Removed comment. |
| 204 ↗ | (On Diff #41904) | Added to backlog via T3270. |
| 229 ↗ | (On Diff #41904) | Corrected to reflect length of the pk_script, which is 25 bytes in length and safe under uint8 (1 byte).  |
| modules/ecash-coinselect/test/utxo.test.js | ||
| 52 ↗ | (On Diff #41904) | Updated verification via online tx size calculator |
| 70 ↗ | (On Diff #41904) | Updated verification via online tx size calculator |
| 88 ↗ | (On Diff #41904) | Updated verification via online tx size calculator |
| modules/ecash-coinselect/src/utxo.js | ||
|---|---|---|
| 31 ↗ | (On Diff #41912) | I don't see anything in that function that checks the utxos are p2pkh |
| 175–176 ↗ | (On Diff #41912) | |
| 222 ↗ | (On Diff #41912) | Why do you link an old version of bitcoin ?? Also this comment is really irrelevant, describing what is not in the code is useless. The relevant part is only the line below |
| 164 ↗ | (On Diff #41904) | it's not updated ? |
| 193 ↗ | (On Diff #41904) | That's not the minimum technically, it's just statistically irrelevant to consider lower signatures |
| modules/ecash-coinselect/test/utxo.test.js | ||
| 52 ↗ | (On Diff #41904) | Do you understand why I said it was wrong in your case ? Having the raw tx hex was much nicer that using a 3rd party lib to check your implementation. I have no idea if that lib is correct. |
Comment Actions
Responding to feedback
| modules/ecash-coinselect/src/utxo.js | ||
|---|---|---|
| 31 ↗ | (On Diff #41912) | reverted |
| 175–176 ↗ | (On Diff #41912) | Updated |
| 222 ↗ | (On Diff #41912) | Removed redundant info |
| 164 ↗ | (On Diff #41904) | Updated for real this time |
| 193 ↗ | (On Diff #41904) | Updated |
| modules/ecash-coinselect/test/utxo.test.js | ||
| 52 ↗ | (On Diff #41904) | Reverted back to raw tx hex. |
| modules/ecash-coinselect/test/utxo.test.js | ||
|---|---|---|
| 52 ↗ | (On Diff #41904) | Except that there is no variance in the scriptpubkey. Where is the variance ? |
| modules/ecash-coinselect/test/utxo.test.js | ||
|---|---|---|
| 52 ↗ | (On Diff #41904) | yup you're right, all values in P2PKH_OUT_SIZE is fixed, I was thinking about FRAMEWORK_BYTES where there is variance based on the amount of inputs and outputs. |
| modules/ecash-coinselect/test/utxo.test.js | ||
|---|---|---|
| 52 ↗ | (On Diff #41904) |
Yes, the only variance is the signature size, but it can't be 73 bytes (because you're accounting for the sighash byte separately). Your way of counting is to use the worst case as per fee computation, i.e. the max possible size. So your evaluation CANNOT be less than the real size, it can only be over-evaluated. And realistically there is very little chance than you get a signature with a size which is not 71 or 72 bytes (unless it's a schnorr signature, but they are still rare). So in your case it's fair to consider that you can over-evaluate by 1 byte per input (but not under-evaluate, which was why I told you it was wrong to begin with). Please make it very clear in a comment why you can have this error and why you choose that range. |
Comment Actions
It looks good now, so the only thing that remains is to get rid of the boilerplate. Create a function that takes the raw tx in the test to avoid repeating the comment and the asserts all over the place, and it's good to go.
Comment Actions
Ok, can you please split this in 2 pieces ? The actual byte count bug fix first, which should be green easily as it's been heavily reviewed here, then the input accounting for op_return in another diff based on this ? This diff has more to do with the byte count calculation than the feature change now. Also please update the title and summary accordingly.
Comment Actions
Code lgtm but I see there is not test on CI, can you please add the CI config for this lib ? In another diff ofc