search

lbryio / lbrycrd

The blockchain that provides the digital content namespace for the LBRY protocol
https://lbry.com
MIT License
2.57k stars 178 forks source link

Specify non standard tx errors #185

Closed bvbfan closed 6 years ago

bvbfan commented 6 years ago

Signed-off-by: Anthony Fieroni bvbfan@abv.bg

bvbfan commented 6 years ago

To pass clang-format-diff.py should make an unrelated comment change in src/policy/policy.h

BrannonKing commented 6 years ago

Was there an existing issue reported for this?

bvbfan commented 6 years ago

https://github.com/lbryio/lbrycrd/issues/97

lbrynaut commented 6 years ago

Just noting here that this is rather invasive (affecting consensus code; i.e. anything in consensus directory), and potentially breaks forward/upstream compatibility in perpetuity. It's also another good example of over-engineering where not necessary. IMO, the correct solution is to do what upstream did -- remove the unused constant. Since it's just a matter of error reporting, replacing the perfectly good reason string with codes in this PR is complete overkill.

I don't agree with the comment from @kaykurokawa from the issue (and I suspect he would reconsider if he knew what upstream did), but even if I did, this PR should be maybe 2 lines long.

bvbfan commented 6 years ago

@lbrynaut , @kaykurokawa i can simplify patch as

if (reason == "dust") 
    return state.DoS(0, false, REJECT_DUST, reason);

Other remains unchanged.

kaykurokawa commented 6 years ago

I agree that this is a bit too much, which will affect upstream compatibility without a good reason. FYI, the reject codes here is based on BIP 61: https://github.com/bitcoin/bips/blob/master/bip-0061.mediawiki

I think the suggestion to add below line to AcceptToMemoryPoolWorker is fine:

if (reason == "dust") 
    return state.DoS(0, false, REJECT_DUST, reason);
bvbfan commented 6 years ago

Implemented as you suggest.

kaykurokawa commented 6 years ago

LGTM