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

Upgrade polkadot-sdk to stable2412 #1595

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

0xbillw
Copy link

@0xbillw 0xbillw commented Jan 18, 2025

The main reason for the upgrade is to try to resolve the memory leak issue in polkadot-sdk stable2409, but this PR has not been backported to stable2409.

Cargo.toml Outdated Show resolved Hide resolved
Cargo.toml Outdated Show resolved Hide resolved
@0xbillw
Copy link
Author

0xbillw commented Jan 18, 2025

Now, it's blocked as this rust-ethereum/evm#305

@0xbillw 0xbillw force-pushed the stable2412 branch 2 times, most recently from b9954f4 to 5ae383a Compare January 21, 2025 04:17
@boundless-forest
Copy link
Collaborator

Please merge master and fix those tests. Thanks!

@0xbillw
Copy link
Author

0xbillw commented Jan 21, 2025

I am struggling with integration testing and I estimate it will take me some time to complete the work.

Copy link
Contributor

@conr2d conr2d left a comment

Choose a reason for hiding this comment

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

I leave comments on parts where Deref is not necessarily required. As for the SelfContained, I haven’t been able to review it in depth yet, but I will take the time to revisit it.

@@ -147,7 +147,7 @@ where
graph
.validated_pool()
.ready()
.map(|in_pool_tx| in_pool_tx.data().clone())
.map(|in_pool_tx| in_pool_tx.data().deref().clone())
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
.map(|in_pool_tx| in_pool_tx.data().deref().clone())
.map(|in_pool_tx| in_pool_tx.data().as_ref().clone())

@@ -16,7 +16,7 @@
// You should have received a copy of the GNU General Public License
// along with this program. If not, see <https://www.gnu.org/licenses/>.

use std::sync::Arc;
use std::{ops::Deref, sync::Arc};
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
use std::{ops::Deref, sync::Arc};
use std::sync::Arc;

@@ -157,7 +157,7 @@ where
.validated_pool()
.futures()
.iter()
.map(|(_hash, extrinsic)| extrinsic.clone())
.map(|(_hash, extrinsic)| extrinsic.deref().clone())
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
.map(|(_hash, extrinsic)| extrinsic.deref().clone())
.map(|(_hash, extrinsic)| extrinsic.as_ref().clone())

@@ -19,6 +19,7 @@
use std::{
collections::{BTreeMap, HashSet},
marker::PhantomData,
ops::Deref,
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
ops::Deref,

@@ -111,7 +112,7 @@ where
.graph
.validated_pool()
.ready()
.map(|in_pool_tx| in_pool_tx.data().clone())
.map(|in_pool_tx| in_pool_tx.data().deref().clone())
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
.map(|in_pool_tx| in_pool_tx.data().deref().clone())
.map(|in_pool_tx| in_pool_tx.data().as_ref().clone())

@@ -16,7 +16,7 @@
// You should have received a copy of the GNU General Public License
// along with this program. If not, see <https://www.gnu.org/licenses/>.

use std::{marker::PhantomData, sync::Arc};
use std::{marker::PhantomData, ops::Deref, sync::Arc};
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
use std::{marker::PhantomData, ops::Deref, sync::Arc};
use std::{marker::PhantomData, sync::Arc};

@@ -109,7 +109,7 @@ where
.graph
.validated_pool()
.ready()
.map(|in_pool_tx| in_pool_tx.data().clone())
.map(|in_pool_tx| in_pool_tx.data().deref().clone())
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
.map(|in_pool_tx| in_pool_tx.data().deref().clone())
.map(|in_pool_tx| in_pool_tx.data().as_ref().clone())

@@ -118,7 +118,7 @@ where
.validated_pool()
.futures()
.iter()
.map(|(_, extrinsic)| extrinsic.clone())
.map(|(_, extrinsic)| extrinsic.deref().clone())
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
.map(|(_, extrinsic)| extrinsic.deref().clone())
.map(|(_, extrinsic)| extrinsic.as_ref().clone())

Comment on lines 126 to +127
let tmp: &mut [u8; 32] = &mut [0; 32];
index.to_big_endian(tmp);
index.write_as_big_endian(tmp);
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of doing this, you can remove all these and update LINE 131:

-		key.extend(blake2_128_extend(tmp));
+ 		key.extend(blake2_128_extend(&index.to_big_endian()));

@@ -282,8 +282,7 @@ impl Precompile for Bn128Pairing {
}
};

let mut buf = [0u8; 32];
ret_val.to_big_endian(&mut buf);
let buf = ret_val.to_big_endian();
Copy link
Contributor

Choose a reason for hiding this comment

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

What about updating LINE 290 instead of assigning a new buf here?

		Ok(PrecompileOutput {
			exit_status: ExitSucceed::Returned,
-			output: buf.to_vec(),
+			output: ret_val.to_big_endian().to_vec(),
		})

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.

4 participants