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

Bug in compilation when multiple versions are used with many contracts #4550

Merged
merged 7 commits into from
Nov 7, 2023
Merged

Bug in compilation when multiple versions are used with many contracts #4550

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
279238c
first version example
ChristopherDedominici Oct 31, 2023
370ebb5
add mutex explanation + add tests
ChristopherDedominici Nov 1, 2023
049d521
Create changeset small-crews-kiss.md
ChristopherDedominici Nov 2, 2023
67a2db1
replace array with variable
ChristopherDedominici Nov 2, 2023
99317e6
Merge branch 'bug/4490-bug-in-compilation-when-multiple-versions-are-…
ChristopherDedominici Nov 2, 2023
9921c1d
add test to check that callbacks are called
ChristopherDedominici Nov 3, 2023
5d211ba
modify callbacks comments
ChristopherDedominici Nov 3, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
73 changes: 37 additions & 36 deletions packages/hardhat-core/src/builtin-tasks/compile.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -580,26 +580,26 @@ subtask(TASK_COMPILE_SOLIDITY_GET_SOLC_BUILD)
compilersCache
);

const isCompilerDownloaded = await downloader.isCompilerDownloaded(
solcVersion
await downloader.downloadCompiler(
solcVersion,
// downloadStartedCb
async (isCompilerDownloaded: boolean) => {
await run(TASK_COMPILE_SOLIDITY_LOG_DOWNLOAD_COMPILER_START, {
solcVersion,
isCompilerDownloaded,
quiet,
});
},
// downloadEndedCb
async (isCompilerDownloaded: boolean) => {
await run(TASK_COMPILE_SOLIDITY_LOG_DOWNLOAD_COMPILER_END, {
solcVersion,
isCompilerDownloaded,
quiet,
});
}
);

if (!isCompilerDownloaded) {
await run(TASK_COMPILE_SOLIDITY_LOG_DOWNLOAD_COMPILER_START, {
solcVersion,
isCompilerDownloaded,
quiet,
});

await downloader.downloadCompiler(solcVersion);

await run(TASK_COMPILE_SOLIDITY_LOG_DOWNLOAD_COMPILER_END, {
solcVersion,
isCompilerDownloaded,
quiet,
});
}

const compiler = await downloader.getCompiler(solcVersion);

if (compiler !== undefined) {
Expand All @@ -615,24 +615,25 @@ subtask(TASK_COMPILE_SOLIDITY_GET_SOLC_BUILD)
compilersCache
);

const isWasmCompilerDownloader =
await wasmDownloader.isCompilerDownloaded(solcVersion);

if (!isWasmCompilerDownloader) {
await run(TASK_COMPILE_SOLIDITY_LOG_DOWNLOAD_COMPILER_START, {
solcVersion,
isCompilerDownloaded,
quiet,
});

await wasmDownloader.downloadCompiler(solcVersion);

await run(TASK_COMPILE_SOLIDITY_LOG_DOWNLOAD_COMPILER_END, {
solcVersion,
isCompilerDownloaded,
quiet,
});
}
await wasmDownloader.downloadCompiler(
solcVersion,
async (isCompilerDownloaded: boolean) => {
// downloadStartedCb
await run(TASK_COMPILE_SOLIDITY_LOG_DOWNLOAD_COMPILER_START, {
solcVersion,
isCompilerDownloaded,
quiet,
});
},
// downloadEndedCb
async (isCompilerDownloaded: boolean) => {
await run(TASK_COMPILE_SOLIDITY_LOG_DOWNLOAD_COMPILER_END, {
solcVersion,
isCompilerDownloaded,
quiet,
});
}
);

const wasmCompiler = await wasmDownloader.getCompiler(solcVersion);

Expand Down
26 changes: 24 additions & 2 deletions packages/hardhat-core/src/internal/solidity/compiler/downloader.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,11 @@ export interface ICompilerDownloader {
* Downloads the compiler for a given version, which can later be obtained
* with getCompiler.
*/
downloadCompiler(version: string): Promise<void>;
downloadCompiler(
version: string,
downloadStartedCb: (isCompilerDownloaded: boolean) => Promise<any>,
downloadEndedCb: (isCompilerDownloaded: boolean) => Promise<any>
): Promise<void>;

/**
* Returns the compiler, which MUST be downloaded before calling this function.
Expand Down Expand Up @@ -146,8 +150,24 @@ export class CompilerDownloader implements ICompilerDownloader {
return fsExtra.pathExists(downloadPath);
}

public async downloadCompiler(version: string): Promise<void> {
public async downloadCompiler(
version: string,
downloadStartedCb: (isCompilerDownloaded: boolean) => Promise<any>,
downloadEndedCb: (isCompilerDownloaded: boolean) => Promise<any>
): Promise<void> {
// Since only one process at a time can acquire the mutex, we avoid the risk of downloading the same compiler multiple times.
// This is because the mutex blocks access until a compiler has been fully downloaded, preventing any new process
// from checking whether that version of the compiler exists. Without mutex it might incorrectly
// return false, indicating that the compiler isn't present, even though it is currently being downloaded.
Copy link
Member

Choose a reason for hiding this comment

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

thanks for this!

await this._mutex.use(async () => {
const isCompilerDownloaded = await this.isCompilerDownloaded(version);

if (isCompilerDownloaded === true) {
return;
}

await downloadStartedCb(isCompilerDownloaded);

let build = await this._getCompilerBuild(version);

if (build === undefined && (await this._shouldDownloadCompilerList())) {
Expand Down Expand Up @@ -189,6 +209,8 @@ export class CompilerDownloader implements ICompilerDownloader {
}

await this._postProcessCompilerDownload(build, downloadPath);

await downloadEndedCb(isCompilerDownloaded);
});
}

Expand Down
6 changes: 5 additions & 1 deletion packages/hardhat-core/test/internal/hardhat-network/stack-traces/compilation.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -190,6 +190,10 @@ export async function downloadCompiler(solidityVersion: string) {

if (!isCompilerDownloaded) {
console.log("Downloading solc", solidityVersion);
await downloader.downloadCompiler(solidityVersion);
await downloader.downloadCompiler(
solidityVersion,
async () => {},
async () => {}
);
}
}
150 changes: 136 additions & 14 deletions packages/hardhat-core/test/internal/solidity/compiler/downloader.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,11 @@ describe("Compiler downloader", function () {
});

it("should return true when the compiler is already downloaded", async function () {
await wasmDownloader.downloadCompiler("0.4.12");
await wasmDownloader.downloadCompiler(
"0.4.12",
async () => {},
async () => {}
);
assert.isTrue(await wasmDownloader.isCompilerDownloaded("0.4.12"));
});
});
Expand All @@ -55,20 +59,34 @@ describe("Compiler downloader", function () {
});

it("should return true when the compiler is already downloaded", async function () {
await downloader.downloadCompiler("0.4.12");
await downloader.downloadCompiler(
"0.4.12",
async () => {},
async () => {}
);
assert.isTrue(await downloader.isCompilerDownloaded("0.4.12"));
});
});

describe("downloadCompiler", function () {
it("Should throw if the version is invalid or doesn't exist", async function () {
await expectHardhatErrorAsync(
() => downloader.downloadCompiler("asd"),
() =>
downloader.downloadCompiler(
"asd",
async () => {},
async () => {}
),
ERRORS.SOLC.INVALID_VERSION
);

await expectHardhatErrorAsync(
() => downloader.downloadCompiler("100.0.0"),
() =>
downloader.downloadCompiler(
"100.0.0",
async () => {},
async () => {}
),
ERRORS.SOLC.INVALID_VERSION
);
});
Expand All @@ -84,7 +102,12 @@ describe("Compiler downloader", function () {
);

await expectHardhatErrorAsync(
() => mockDownloader.downloadCompiler("0.4.12"),
() =>
mockDownloader.downloadCompiler(
"0.4.12",
async () => {},
async () => {}
),
ERRORS.SOLC.VERSION_LIST_DOWNLOAD_FAILED
);
});
Expand All @@ -105,7 +128,12 @@ describe("Compiler downloader", function () {
);

await expectHardhatErrorAsync(
() => mockDownloader.downloadCompiler("0.4.12"),
() =>
mockDownloader.downloadCompiler(
"0.4.12",
async () => {},
async () => {}
),
ERRORS.SOLC.DOWNLOAD_FAILED
);
});
Expand All @@ -122,8 +150,16 @@ describe("Compiler downloader", function () {
}
);

await mockDownloader.downloadCompiler("0.4.12");
await mockDownloader.downloadCompiler("0.4.13");
await mockDownloader.downloadCompiler(
"0.4.12",
async () => {},
async () => {}
);
await mockDownloader.downloadCompiler(
"0.4.13",
async () => {},
async () => {}
);

assert.equal(downloads, 3);
});
Expand Down Expand Up @@ -152,17 +188,87 @@ describe("Compiler downloader", function () {
);

await expectHardhatErrorAsync(
() => mockDownloader.downloadCompiler("0.4.12"),
() =>
mockDownloader.downloadCompiler(
"0.4.12",
async () => {},
async () => {}
),
ERRORS.SOLC.INVALID_DOWNLOAD
);

assert.isFalse(await fsExtra.pathExists(compilerPath!));

// it should work with the normal download now
stopMocking = true;
await mockDownloader.downloadCompiler("0.4.12");
await mockDownloader.downloadCompiler(
"0.4.12",
async () => {},
async () => {}
);
assert.isTrue(await mockDownloader.isCompilerDownloaded("0.4.12"));
});

describe("multiple downloads", function () {
it("should not download multiple times the same compiler", async function () {
// The intention is for the value to be 1 if the compiler is downloaded only once.
// Without a mutex, the value would be 10 because the compiler would be downloaded multiple times.
// However, the check is implemented to ensure that the value remains 1.

const VERSION = "0.4.12";

const value = [0];

const promises = [];
for (let i = 0; i < 10; i++) {
promises.push(
downloader.downloadCompiler(
VERSION,
// downloadStartedCb
async () => {
value[0]++;
},
// downloadEndedCb
async () => {}
)
);
}

await Promise.all(promises);

assert.isDefined(downloader.getCompiler(VERSION));
assert(value[0] === 1);
});

it("should download multiple different compilers", async function () {
const VERSIONS = ["0.5.1", "0.5.2", "0.5.3", "0.5.4", "0.5.5"];

const value = [0];

const promises = [];
for (const version of VERSIONS) {
promises.push(
downloader.downloadCompiler(
version,
// downloadStartedCb
async () => {
value[0]++;
},
// downloadEndedCb
async () => {}
)
);
}

await Promise.all(promises);

for (const version of VERSIONS) {
assert.isDefined(downloader.getCompiler(version));
}

assert(value[0] === VERSIONS.length);
});
});
});

describe("getCompiler", function () {
Expand All @@ -174,7 +280,11 @@ describe("Compiler downloader", function () {
});

it("should throw when trying to get a compiler that's in the compiler list but hasn't been downloaded yet", async function () {
await downloader.downloadCompiler("0.4.12");
await downloader.downloadCompiler(
"0.4.12",
async () => {},
async () => {}
);

await expectHardhatErrorAsync(
() => downloader.getCompiler("0.4.13"),
Expand Down Expand Up @@ -223,15 +333,27 @@ describe("Compiler downloader", function () {
}
);

await mockDownloader.downloadCompiler("0.4.12");
await mockDownloader.downloadCompiler(
"0.4.12",
async () => {},
async () => {}
);
assert.isUndefined(await mockDownloader.getCompiler("0.4.12"));
});

it("should work for downloaded compilers", async function () {
await downloader.downloadCompiler("0.4.12");
await downloader.downloadCompiler(
"0.4.12",
async () => {},
async () => {}
);
assert.isDefined(downloader.getCompiler("0.4.12"));

await downloader.downloadCompiler("0.4.13");
await downloader.downloadCompiler(
"0.4.13",
async () => {},
async () => {}
);
assert.isDefined(downloader.getCompiler("0.4.13"));
});
});
Expand Down
Loading
Loading