Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add bitcoing protocol dissector. #1992

Merged
merged 5 commits into from
May 31, 2023
Merged

Add bitcoing protocol dissector. #1992

merged 5 commits into from
May 31, 2023

Conversation

mmaatuq
Copy link
Contributor

@mmaatuq mmaatuq commented May 27, 2023

  • remove bitcoin protcol detection from mining.c
  • add a new bitcoin deissector.
  • add a new category: Cryptocurrency.

@mmaatuq mmaatuq marked this pull request as draft May 27, 2023 15:28
@mmaatuq
Copy link
Contributor Author

mmaatuq commented May 27, 2023

  • This will fail the pipeline as many tests failed.
  • Most of them related to Num dissector calls
  • Once this change is approved tests will be updated.

IvanNardi
IvanNardi requested changes May 27, 2023
View reviewed changes
Copy link
Collaborator

@IvanNardi IvanNardi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall it seems fine. Just some notes:

  • could you update doc/protocols.rst with this new protocol?
  • could you update the unit tests results now? This way it is simpler to check for any regressions...
  • I don't know if there already is a sample of bitcoin traffic in the tests. If not, could you add it, please?
  • missing file in the Windows build...

src/include/ndpi_protocol_ids.h Outdated Show resolved Hide resolved
src/lib/protocols/bitcoin.c Outdated Show resolved Hide resolved
src/lib/protocols/bitcoin.c Outdated Show resolved Hide resolved
src/lib/protocols/bitcoin.c Outdated Show resolved Hide resolved
src/lib/protocols/bitcoin.c Outdated Show resolved Hide resolved
@mmaatuq
Copy link
Contributor Author

mmaatuq commented May 27, 2023

Ok, will add the required changes.
Yes, there bitcoin capture file among tests., will update it along with other tests.

@mmaatuq mmaatuq marked this pull request as ready for review May 28, 2023 12:21
@IvanNardi
Copy link
Collaborator

@utoni, is it fine for you?

@mmaatuq
Copy link
Contributor Author

mmaatuq commented May 30, 2023

@IvanNardi, @utoni,
The "enable_payload_stat" test needs to be updated, this is a new test and it's not on my branch.

  • should I only fetch this test to my branch and update it?
  • or rebase the whole branch to dev and update the test?

@mmaatuq
Add bitcoing protocol dissector.
051cde7 
* remove bitcoin protcol detection from mining.c
* add a new bitcoin deissector.
* add a new category: Cryptocurrency.

Signed-off-by: Mahmoud Maatuq <mahmoudmatook.mm@gmail.com>
@mmaatuq
Remove useless checks and add missing windows and docs file.
00838e7 
Signed-off-by: Mahmoud Maatuq <mahmoudmatook.mm@gmail.com>
@mmaatuq
update affected tests.
b1da406 
Signed-off-by: Mahmoud Maatuq <mahmoudmatook.mm@gmail.com>
@mmaatuq
add a brief version.
87429d5 
Add notes on the difference between normal bitcoin protocol and the
mining protocol.

Signed-off-by: Mahmoud Maatuq <mahmoudmatook.mm@gmail.com>
@mmaatuq
update enable_payload_stat test after dev rebasing.
81248bd 
Signed-off-by: Mahmoud Maatuq <mahmoudmatook.mm@gmail.com>
@sonarcloud
Copy link

sonarcloud bot commented May 31, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@IvanNardi
Copy link
Collaborator

@IvanNardi, @utoni, The "enable_payload_stat" test needs to be updated, this is a new test and it's not on my branch.

* should I only fetch this test to my branch and update it?

* or rebase the whole branch to dev and update the test?

Usually you need to rebase to have your changes on top of latest dev

@IvanNardi IvanNardi merged commit e17fa12 into ntop:dev May 31, 2023
33 checks passed
@IvanNardi
Copy link
Collaborator

Thank you for your contribution

@mmaatuq mmaatuq deleted the bitcoin_dissecotr branch June 3, 2023 12:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@IvanNardi IvanNardi IvanNardi approved these changes

@utoni utoni utoni approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@mmaatuq @IvanNardi @utoni