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.

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

Stop storing duplicate entries in the cache.db network_manager table #2649

Conversation

hewison-chris
Copy link
Contributor

By making the utxo the Primary key of this table we will replace
records if we get an update from the registry. Currently we just keep
adding new records. The side effet of this fix is that we will not store
nodes that do not have a utxo (i.e. fullnodes).
When we start adding more information about peers into our cache we will
need to decide how to handle fullnodes as well as banned and whitelisted
info.

@codecov
Copy link

codecov bot commented Nov 8, 2021

Codecov Report

Merging #2649 (dae6e1d) into v0.x.x (c76ad8f) will increase coverage by 1.47%.
The diff coverage is 100.00%.

❗ Current head dae6e1d differs from pull request most recent head bcff88a. Consider uploading reports for the commit bcff88a to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           v0.x.x    #2649      +/-   ##
==========================================
+ Coverage   87.83%   89.31%   +1.47%     
==========================================
  Files         155      156       +1     
  Lines       15094    15185      +91     
==========================================
+ Hits        13258    13562     +304     
+ Misses       1836     1623     -213     
Flag Coverage Δ
integration 41.97% <100.00%> (?)
unittests 87.87% <100.00%> (+0.03%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
source/agora/network/Manager.d 85.05% <100.00%> (+8.68%) ⬆️
source/agora/consensus/protocol/Nominator.d 90.16% <0.00%> (-0.48%) ⬇️
source/agora/node/main.d 48.88% <0.00%> (ø)
source/agora/script/Engine.d 97.73% <0.00%> (+0.15%) ⬆️
source/agora/consensus/validation/Transaction.d 98.02% <0.00%> (+0.24%) ⬆️
source/agora/node/Validator.d 93.52% <0.00%> (+1.76%) ⬆️
source/agora/node/Config.d 72.58% <0.00%> (+2.41%) ⬆️
source/agora/node/FullNode.d 76.69% <0.00%> (+2.63%) ⬆️
source/agora/test/LocalTransactions.d 90.90% <0.00%> (+4.54%) ⬆️
... and 6 more

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 c76ad8f...bcff88a. Read the comment docs.

@@ -455,7 +455,7 @@ public class NetworkManager

this.cacheDB.execute(
"CREATE TABLE IF NOT EXISTS network_manager (" ~
"utxo TEXT, pubkey TEXT, address TEXT NOT NULL)");
"utxo TEXT PRIMARY KEY, pubkey TEXT, address TEXT NOT NULL)");
Copy link
Contributor

Choose a reason for hiding this comment

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

But a node can have multiple interfaces, therefore multiple addresses. This does not work in that case, no?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Then does it make more sense to have address as the primary key?

Copy link
Contributor Author

@hewison-chris hewison-chris Nov 8, 2021

Choose a reason for hiding this comment

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

Example of cache.db network_manager table on node 5 earlier:

select * from network_manager ;
0xdb7664caba94c8d4602c10992c13176307e1e05361c150217166ee77fc4af9bf176f31dc61aba61e634dfc0b4c5f729d59e604607f61c9f66b10c6841f972a0a|boa1xzval4nvru2ej9m0rptq7hatukkavemryvct4f8smyy3ky9ct5u0s8w6gfy|https://v4.bosagora.io
0x9b2726e79f05abc107b6531486a46c977414e13ed9f3ee994ec14504964f86fcf9464055b891b9c34020feb72535c300ff19e8b5167eb9d202db1a053d746b2c|boa1xrval6hd8szdektyz69fnqjwqfejhu4rvrpwlahh9rhaazzpvs5g6lh34l5|https://v6.bosagora.io
0x7bacc99e9bf827f0fa6dc6a77303d2e6ba6f1591277b896a9305a9e200853986fe9527fd551077a4ac2b511633ada4190a7b82cddaf606171336e1efba87ea8f|boa1xzval3ah8z7ewhuzx6mywveyr79f24w49rdypwgurhjkr8z2ke2mycftv9n|https://v3.bosagora.io
0x3b44d65edb3361dd91441ab4f449eeda55644026624c4b8ae12ecf0264fa8a228dbf672ef97e2c4f87fb98ad7099e17b7f9ba7dbe8479672066912b1ea24ba77|boa1xzval2a3cdxv28n6slr62wlczslk3juvk7cu05qt3z55ty2rlfqfc6egsh2|https://v2.bosagora.io
0x210f6551d648a4da654da116b100e941e434e4f232b8579439c2ef64b04819bd2782eb3524c7a29c38c347cdf26006bccac54a58a58f103ae7eb5b252eb53b64|boa1xrval7gwhjz4k9raqukcnv2n4rl4fxt74m2y9eay6l5mqdf4gntnzhhscrh|https://v7.bosagora.io
0xdb7664caba94c8d4602c10992c13176307e1e05361c150217166ee77fc4af9bf176f31dc61aba61e634dfc0b4c5f729d59e604607f61c9f66b10c6841f972a0a|boa1xzval4nvru2ej9m0rptq7hatukkavemryvct4f8smyy3ky9ct5u0s8w6gfy|https://v4.bosagora.io
0x9b2726e79f05abc107b6531486a46c977414e13ed9f3ee994ec14504964f86fcf9464055b891b9c34020feb72535c300ff19e8b5167eb9d202db1a053d746b2c|boa1xrval6hd8szdektyz69fnqjwqfejhu4rvrpwlahh9rhaazzpvs5g6lh34l5|https://v6.bosagora.io
0x210f6551d648a4da654da116b100e941e434e4f232b8579439c2ef64b04819bd2782eb3524c7a29c38c347cdf26006bccac54a58a58f103ae7eb5b252eb53b64|boa1xrval7gwhjz4k9raqukcnv2n4rl4fxt74m2y9eay6l5mqdf4gntnzhhscrh|https://v7.bosagora.io
0x7bacc99e9bf827f0fa6dc6a77303d2e6ba6f1591277b896a9305a9e200853986fe9527fd551077a4ac2b511633ada4190a7b82cddaf606171336e1efba87ea8f|boa1xzval3ah8z7ewhuzx6mywveyr79f24w49rdypwgurhjkr8z2ke2mycftv9n|https://v3.bosagora.io
0x3b44d65edb3361dd91441ab4f449eeda55644026624c4b8ae12ecf0264fa8a228dbf672ef97e2c4f87fb98ad7099e17b7f9ba7dbe8479672066912b1ea24ba77|boa1xzval2a3cdxv28n6slr62wlczslk3juvk7cu05qt3z55ty2rlfqfc6egsh2|https://v2.bosagora.io
0xdb7664caba94c8d4602c10992c13176307e1e05361c150217166ee77fc4af9bf176f31dc61aba61e634dfc0b4c5f729d59e604607f61c9f66b10c6841f972a0a|boa1xzval4nvru2ej9m0rptq7hatukkavemryvct4f8smyy3ky9ct5u0s8w6gfy|https://v4.bosagora.io
0x210f6551d648a4da654da116b100e941e434e4f232b8579439c2ef64b04819bd2782eb3524c7a29c38c347cdf26006bccac54a58a58f103ae7eb5b252eb53b64|boa1xrval7gwhjz4k9raqukcnv2n4rl4fxt74m2y9eay6l5mqdf4gntnzhhscrh|https://v7.bosagora.io
0x3b44d65edb3361dd91441ab4f449eeda55644026624c4b8ae12ecf0264fa8a228dbf672ef97e2c4f87fb98ad7099e17b7f9ba7dbe8479672066912b1ea24ba77|boa1xzval2a3cdxv28n6slr62wlczslk3juvk7cu05qt3z55ty2rlfqfc6egsh2|https://v2.bosagora.io
0x7bacc99e9bf827f0fa6dc6a77303d2e6ba6f1591277b896a9305a9e200853986fe9527fd551077a4ac2b511633ada4190a7b82cddaf606171336e1efba87ea8f|boa1xzval3ah8z7ewhuzx6mywveyr79f24w49rdypwgurhjkr8z2ke2mycftv9n|https://v3.bosagora.io
0x9b2726e79f05abc107b6531486a46c977414e13ed9f3ee994ec14504964f86fcf9464055b891b9c34020feb72535c300ff19e8b5167eb9d202db1a053d746b2c|boa1xrval6hd8szdektyz69fnqjwqfejhu4rvrpwlahh9rhaazzpvs5g6lh34l5|https://v6.bosagora.io
0xdb7664caba94c8d4602c10992c13176307e1e05361c150217166ee77fc4af9bf176f31dc61aba61e634dfc0b4c5f729d59e604607f61c9f66b10c6841f972a0a|boa1xzval4nvru2ej9m0rptq7hatukkavemryvct4f8smyy3ky9ct5u0s8w6gfy|https://v4.bosagora.io
0x210f6551d648a4da654da116b100e941e434e4f232b8579439c2ef64b04819bd2782eb3524c7a29c38c347cdf26006bccac54a58a58f103ae7eb5b252eb53b64|boa1xrval7gwhjz4k9raqukcnv2n4rl4fxt74m2y9eay6l5mqdf4gntnzhhscrh|https://v7.bosagora.io
0x210f6551d648a4da654da116b100e941e434e4f232b8579439c2ef64b04819bd2782eb3524c7a29c38c347cdf26006bccac54a58a58f103ae7eb5b252eb53b64|boa1xrval7gwhjz4k9raqukcnv2n4rl4fxt74m2y9eay6l5mqdf4gntnzhhscrh|https://v6.bosagora.io
0x7bacc99e9bf827f0fa6dc6a77303d2e6ba6f1591277b896a9305a9e200853986fe9527fd551077a4ac2b511633ada4190a7b82cddaf606171336e1efba87ea8f|boa1xzval3ah8z7ewhuzx6mywveyr79f24w49rdypwgurhjkr8z2ke2mycftv9n|https://v3.bosagora.io
0x3b44d65edb3361dd91441ab4f449eeda55644026624c4b8ae12ecf0264fa8a228dbf672ef97e2c4f87fb98ad7099e17b7f9ba7dbe8479672066912b1ea24ba77|boa1xzval2a3cdxv28n6slr62wlczslk3juvk7cu05qt3z55ty2rlfqfc6egsh2|https://v2.bosagora.io
0x210f6551d648a4da654da116b100e941e434e4f232b8579439c2ef64b04819bd2782eb3524c7a29c38c347cdf26006bccac54a58a58f103ae7eb5b252eb53b64|boa1xrval7gwhjz4k9raqukcnv2n4rl4fxt74m2y9eay6l5mqdf4gntnzhhscrh|https://v6.bosagora.io
0x210f6551d648a4da654da116b100e941e434e4f232b8579439c2ef64b04819bd2782eb3524c7a29c38c347cdf26006bccac54a58a58f103ae7eb5b252eb53b64|boa1xrval7gwhjz4k9raqukcnv2n4rl4fxt74m2y9eay6l5mqdf4gntnzhhscrh|https://v7.bosagora.io
0xdb7664caba94c8d4602c10992c13176307e1e05361c150217166ee77fc4af9bf176f31dc61aba61e634dfc0b4c5f729d59e604607f61c9f66b10c6841f972a0a|boa1xzval4nvru2ej9m0rptq7hatukkavemryvct4f8smyy3ky9ct5u0s8w6gfy|https://v4.bosagora.io
0x3b44d65edb3361dd91441ab4f449eeda55644026624c4b8ae12ecf0264fa8a228dbf672ef97e2c4f87fb98ad7099e17b7f9ba7dbe8479672066912b1ea24ba77|boa1xzval2a3cdxv28n6slr62wlczslk3juvk7cu05qt3z55ty2rlfqfc6egsh2|https://v2.bosagora.io
0x7bacc99e9bf827f0fa6dc6a77303d2e6ba6f1591277b896a9305a9e200853986fe9527fd551077a4ac2b511633ada4190a7b82cddaf606171336e1efba87ea8f|boa1xzval3ah8z7ewhuzx6mywveyr79f24w49rdypwgurhjkr8z2ke2mycftv9n|https://v3.bosagora.io

Copy link
Contributor Author

@hewison-chris hewison-chris Nov 8, 2021

Choose a reason for hiding this comment

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

^ See also a case where v6 has public key of val7.
vibe.d bug #1395 (comment) again?

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's have address as the primary key, and if we can't connect to that address or utxo/pubkey has changed since we wrote it to the DB, we can just delete that record.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Geod24 better to drop the protocol from the address we store?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

^^ I like the idea of dropping a record which fails also.

By making all the columns part of the Primary key of this table we will
replace records if they already exist. Currently we just keep adding
new records.
@hewison-chris hewison-chris force-pushed the network_manager_duplicates branch from dae6e1d to bcff88a Compare November 8, 2021 08:05
@hewison-chris hewison-chris changed the title Only store single entry in cache.db network_manager table Stop storing duplicate entries in the cache.db network_manager table Nov 8, 2021
@hewison-chris
Copy link
Contributor Author

I am going to convert this to draft for now until I understand how the table is used and whether we need to refactor a bit more before making changes.

@hewison-chris hewison-chris marked this pull request as draft November 8, 2021 08:09
@hewison-chris
Copy link
Contributor Author

After analyzing the code I see that having duplicate entries here will not actually cause any harm as only the addresses are read on startup into sets which can not store duplicates.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants