Separate all bitcoinconsensus validation stuff in module

[stevenroose]

Includes #327.

Not sure if it's the right place to put the module, but I think it makes
more sense to have all validation stuff inside the same module and have
a validation::Error type for validation methods.

[tamasblummer]

if you are fancy this could also have functions

• to do locktime check, asking if a transaction is valid to be included into a block with given height or under assumption of an block time average.
• to check if the transaction has no output less than DUST limit
• pays at least x sats/vByte

[elichai]

I think you forgot to commit the validation.rs file. :)

[stevenroose]

Haha, of course I did! Fixed.

[tamasblummer]

@stevenroose this does not compile, please fix or close

[stevenroose]

updated

[codecov-io]

Codecov Report

Merging #330 into master will increase coverage by 0.39%.
The diff coverage is 78.63%.

@@            Coverage Diff             @@
##           master     #330      +/-   ##
==========================================
+ Coverage   82.17%   82.56%   +0.39%     
==========================================
  Files          38       39       +1     
  Lines        7063     7510     +447     
==========================================
+ Hits         5804     6201     +397     
- Misses       1259     1309      +50

Impacted Files	Coverage Δ
src/blockdata/script.rs	81.67% <ø> (+3.08%)	⬆️
src/lib.rs	100% <ø> (ø)	⬆️
src/blockdata/transaction.rs	94.12% <ø> (+0.64%)	⬆️
src/consensus/validation.rs	78.63% <78.63%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4b1d4ed...25fbea0. Read the comment docs.

[apoelstra]

Needs rebase.

concept ACK.

@stevenroose one thing I like about this is that it removes #[cfg(...)] markers on enum variants. I think these are problematic because it makes it a breaking change to enable/disable features, for users who are matching on these enums. Because of how cargo resolves features, it means that a user might find their build breaks when they add an unrelated dep.

In #413 you add some new instances of feature-gated enum variants. I wonder if we should change that too?

[stevenroose]

Rebased and updated this PR.

[apoelstra]

ack 310f9926049cd379675c242154f7d37f3cda0fc4

[JeremyRubin]

I think it might be a superior approach for compatibility to implement the functions in the validation module on the type rather than as standalone funcs.

e.g.

pub struct Foo;
 impl Foo {
     pub fn foo(){
         println!("foo");
 
     }
 }
 
 mod extend {
     impl super::Foo {
         pub fn bar() {
             println!("bar");
 
         }
     }
 }
 fn main() {
     Foo::bar();
     Foo::foo();
 }

[stevenroose]

Hmm, in what way do you mean "for compatibility"?

[JeremyRubin]

the method names do not change, nor does how you call them. less impact on the crate's current users.

[stevenroose]

Oh yeah this PR is so old I forgot it removes existing methods. How about adding them so that we have the bare module functions and the existing methods that refer to them?

…

On Sat, Oct 31, 2020, 23:19 Jeremy Rubin ***@***.*** wrote: the method names do not change, nor does how you call them. less impact on the crate's current users. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#330 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAGQLXCFD2DXRBR3HX2FXADSNSLRDANCNFSM4IWTCQHA> .

[JeremyRubin]

not sure what you're suggesting -- it's fine that this is a breaking change w.r.t. a feature flag on the crate, I'm more pointing out that you can just move the impls to a split out feature module and not rename or remove any exisiting method.

[stevenroose]

@JeremyRubin I added the methods you suggested. Now they're duplicated with the raw module functions though. I somehow feel that it makes sense to bundle validation in one module and just have it there as module functions. Perhaps I should mark the type methods as deprecated? Thoughts? @apoelstra @elichai @sgeisler ?

[elichai]

Concept ACK for splitting,
I don't really like that now impl Transaction and others are scattered in multiple modules, I'd rather have a Trait Validation if we really want to have methods on the objects themselved

[JeremyRubin]

The issue with trait Validation is there isn't a consistent API on the methods -- you could do trait TransactionValidation though?

@stevenroose I guess I was saying that I don't see a reason to introduce the non type methods at all? Is this a general direction we're going or is there a plan to make this a separate crate?

[tcharding]

Hey @stevenroose I like the purpose of this PR, do you want me to redo it? Just offering because it will be way easier for me to do it from scratch knowing whats changed in the codebase since you originally did this than you trying to rebase.

[stevenroose]

Yeah sure, go ahead use whatever I put here.

[tcharding]

Done in #1912