Fixing Config endpoint and adding preferred peers by davidtavarez · Pull Request #196 · GrinPlusPlus/GrinPlusPlus

davidtavarez · 2022-02-09T17:18:08Z

Findings: Socket (P2P) instability while syncing

DavidBurkett · 2022-02-09T17:21:21Z

src/API/Node/Handlers/GetConfigHandler.h

 		config_json["min_confirmations"] = config.GetMinimumConfirmations();

-		return request.BuildResult(config_json);
+		result["Ok"] = config_json;


Why this? The "Ok" response is completely redundant, and goes against the jsonrpc 2.0 spec. I'm aware that the rust repo does that, but it's completely wrong to do so.

true, you're correct 👍🏽

DavidBurkett · 2022-02-09T17:24:44Z

src/Server/Main.cpp

 	{
 		pConfig = Config::Load(config_path, environment);
+		if(config_path.has_value()){
+			pConfig->SetConfigPath(config_path.value());


It's technically breaking the pattern by including config path in the config itself, since it's not actually part of the config. If you're going to though, you should at least move the calls to these setters into Config::Load, so other consumers get it for free. Also, this allows you to avoid duplicating the path calculation logic on lines 72-73

could src\Core\Global.cpp be a better place? let's not break the pattern.

Yep, 2 options come to mind:

Store it as a new field in global with a getter/setter

Wrap ConfigPtr with a new class (ConfigFile? LoadedConfig?) that has 2 fields: path & values(the ConfigPtr). Then store that in global instead of the ConfigPtr.

DavidBurkett · 2022-02-11T00:39:21Z

src/Core/Config.cpp

 uint8_t Config::GetMinSyncPeers() const noexcept { return m_pImpl->m_nodeConfig.GetP2P().GetMinSyncPeers(); }

+const std::vector<std::string>& Config::GetPreferredPeers() const noexcept { return m_pImpl->m_nodeConfig.GetP2P().GetPreferredPeers(); }
+const std::vector<std::string>& Config::GetAlloweddPeers() const noexcept { return m_pImpl->m_nodeConfig.GetP2P().GetAllowedPeers(); }


I'm a fan of double D's, but not at the end of the word 'allowed'

DavidBurkett · 2022-02-11T00:41:26Z

src/Core/Config/P2PConfig.h

+				Json::Value peers = p2pJSON.get(ConfigProps::P2P::PREFERRED_PEERS, Json::Value(Json::nullValue));
+				for (auto& peer : peers) {
+					m_peferredPeers.push_back(peer.asString());
+                }


davidtavarez · 2022-02-11T00:42:22Z

While testing, I'm constantly getting this error from the peers:

Error: The I/O operation has been aborted because of either a thread exit or an application request.

I would appreciate suggestions on how to properly debug that.

DavidBurkett · 2022-02-11T00:43:47Z

src/P2P/Seed/Seeder.cpp

        ));
+
+        if(Global::GetConfig().GetBlockedPeers().size() > 0 && 
+            Global::GetConfig().IsPeerBlocked(pSocket->GetIPAddress().GetAddress().to_string()))


No need to check size first

DavidBurkett · 2022-02-11T00:44:18Z

src/P2P/Seed/Seeder.cpp

+            return;
+        }

+        if(Global::GetConfig().GetAlloweddPeers().size() > 0 && 


IsPeerAllowed should do the empty check

DavidBurkett

Couple small comments, but other than that, looks good

DavidBurkett · 2022-02-11T00:48:47Z

While testing, I'm constantly getting this error from the peers:

Error: The I/O operation has been aborted because of either a thread exit or an application request.

I would appreciate suggestions on how to properly debug that.

I'll message you on TG tmrw

DavidBurkett · 2022-02-11T23:39:55Z

src/Core/Config.cpp

 	return false;
 }

+bool Config::IsPeerPreferred (const std::string peer) {


std::string => std::string&

nit: Unnecessary space between IsPeerPreferred and (

Better yet, take in a const IPAddress& instead, and store IPAddresss when parsing the config.

DavidBurkett · 2022-02-11T23:43:37Z

src/Core/Config.cpp

 }

+bool Config::IsPeerPreferred (const std::string peer) {
+	std::vector<std::string> vec = m_pImpl->m_nodeConfig.GetP2P().GetPreferredPeers();


You need to store this variable as a const std::vector<std::string>& , or else this will be doing an expensive copy on every single usage.

Also, since you're always treating preferred peers as a set, you should be storing it as one (actually, a std::unordered_set, which tends to have an even faster lookup time)

DavidBurkett · 2022-02-11T23:45:55Z

src/P2P/Seed/Seeder.cpp

+
+            const std::string& ipAddressStr = ipAddress.GetAddress().to_string();
+
+            if(Global::GetConfig().IsPeerBlocked(ipAddressStr)) {


nit: C++ devs nearly always include a space between if and (. I would recommend sticking with that formatting standard.

DavidBurkett · 2022-02-11T23:51:20Z

src/P2P/Seed/Seeder.cpp


-        if(Global::GetConfig().IsPeerBlocked(pSocket->GetIPAddress().GetAddress().to_string())) {
-            LOG_TRACE_F("peer is blocked: {}", pPeer);
+        const std::string& ipAddressStr = pSocket->GetIPAddress().GetAddress().to_string();


to_string() returns a std::string not a const std::string&, so you shouldn't store it as a reference (I'm surrprised there's no compiler warning). If you follow my suggestion to change the IsPeerXXX functions to take in a const IPAddress& instead, then you can just store pSocket->GetIPAddress() instead, which does return a const reference.

davidtavarez · 2022-02-16T19:47:33Z

File used to test this PR:

{
	"DATA_PATH" : "D:\\Downloads\\Grin",
	"P2P" : 
	{
		"MAX_PEERS" : 6,
		"MIN_PEERS" : 5,
		"PREFERRED_PEERS" : 
		[
			"217.160.242.228",
			"217.160.66.118",
			"212.227.210.43",
			"93.90.194.26",
			"82.165.107.169",
			"107.174.63.210",
			"grinpp01.grinnode.live",
			"grinpp02.grinnode.live",
			"grinpp03.grinnode.live"
		]
	},
	"WALLET" : 
	{
		"DATABASE" : "SQLITE",
		"MIN_CONFIRMATIONS" : 10
	}
}

…s, and more

davidtavarez · 2022-04-16T11:31:52Z

@DavidBurkett would you mind to do a final review to this PR. I don't I will add nothing else to this PR. I've been testing on Windows, Linux and MacOS.

128kb as buffet size for downloading the TxHashSet file (

GrinPlusPlus/src/P2P/Pipeline/TxHashSetPipe.cpp

Line 17 in 7b43020

static const int BUFFER_SIZE = 256 * 1024;

) seems to be reasonably to me due the amount of slow peers, but I'm not fully convinced yet.

- Setting BUFFER_SIZE = 128 * 1024 for TxHashSetPipe - Checking if tor binary is present to avoid logging unnecessarily.

David Tavarez added 2 commits February 7, 2022 10:48

adding m_peferredPeers to P2PConfig

bcb185b

closes #195

88c5793

davidtavarez self-assigned this Feb 9, 2022

DavidBurkett reviewed Feb 9, 2022

View reviewed changes

David Tavarez added 4 commits February 9, 2022 20:18

fixing return on config handler

0be00f8

Adding options: PREFERRED_PEERS, ALLOWED_PEERS and BLOCKED_PEERS

413b188

moving config file path reference to Global

78fad7a

fixing IP parsing

91786a6

davidtavarez marked this pull request as ready for review February 11, 2022 00:37

davidtavarez requested a review from DavidBurkett February 11, 2022 00:37

DavidBurkett reviewed Feb 11, 2022

View reviewed changes

DavidBurkett approved these changes Feb 11, 2022

View reviewed changes

David Tavarez added 2 commits February 11, 2022 01:54

addressing feedback

e3c699a

limiting pool of peers

f85835f

DavidBurkett reviewed Feb 11, 2022

View reviewed changes

David Tavarez added 2 commits February 16, 2022 12:10

supporting domain names in preferred peers list

dbf3a67

addressing feedback

fdd7bb7

davidtavarez requested a review from DavidBurkett February 16, 2022 14:47

adding IP to the list after resolving

1f371a3

closes #197

3474dc3

davidtavarez mentioned this pull request Feb 16, 2022

Socket (P2P) instability while syncing #198

Closed

David Tavarez added 2 commits February 17, 2022 10:34

capturing exception

6f048db

reducing m_minSyncPeers, socket changes, changing connections timeout…

9f0f930

…s, and more

davidtavarez force-pushed the config/preferred-peers branch from 478b25d to 9f0f930 Compare April 5, 2022 19:05

actions/cache@v2

de66615

davidtavarez force-pushed the config/preferred-peers branch from 0862e86 to de66615 Compare April 5, 2022 20:00

using commit instead of release tag

2a38d12

davidtavarez force-pushed the config/preferred-peers branch from de66615 to 9f0f930 Compare April 10, 2022 15:00

David Tavarez and others added 9 commits April 10, 2022 17:11

updating hash

25ce44f

TxHashSet download time out to 10 minutes

52d4462

restoring time out values

d9bd6c5

tor process stop

f558130

check received data

3af17c3

download time out set to 30 minutes

ef833fd

using asio::read

7796336

using read_some and increasting timeout

ed442b1

improving P2P stability

0bdac88

davidtavarez added 9 commits April 18, 2022 21:11

reducing hashset buffer size and checking if tor binary is found

33b66f9

using FileUtil to check tor binary

294a7eb

moving check

265b617

adding sleep

7b5d43a

adding -Os flag to ci

ee98d5c

removing Os flag from ci

2d39492

adding os detention to know tor extension

2d5d597

setting BUFFER_SIZE to 128 KB

4d412da

Merge pull request #201 from GrinPlusPlus/tests-p2p

a5ec3a6

- Setting BUFFER_SIZE = 128 * 1024 for TxHashSetPipe - Checking if tor binary is present to avoid logging unnecessarily.

davidtavarez merged commit 60fe568 into master Apr 19, 2022

davidtavarez deleted the config/preferred-peers branch April 19, 2022 18:51


		const std::string& ipAddressStr = ipAddress.GetAddress().to_string();

		if(Global::GetConfig().IsPeerBlocked(ipAddressStr)) {

Conversation

davidtavarez commented Feb 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davidtavarez commented Feb 11, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DavidBurkett left a comment

Choose a reason for hiding this comment

Uh oh!

DavidBurkett commented Feb 11, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DavidBurkett Feb 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davidtavarez commented Feb 16, 2022

Uh oh!

davidtavarez commented Apr 16, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

davidtavarez commented Feb 9, 2022 •

edited

Loading

DavidBurkett Feb 11, 2022 •

edited

Loading