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

Added changes to handle default slasher broadcast. #4353

Closed

Conversation

Gua00va
Copy link
Contributor

@Gua00va Gua00va commented May 30, 2023

Issue Addressed

This PR addresses issue #4350

Proposed Changes

This change will enable slasher broadcast by default.

Additional Info

This change will enable slasher broadcast in the following cases:
No flag is passed,
--slasher-broadcast is passed and,
--slasher-broadcast=true is passed.

Only when an explicit false value is passed the slasher does not broadcast.(--slasher-broadcast=false).

TODO

  • Modify CLI parsing logic
  • Write test

@CLAassistant
Copy link

CLAassistant commented May 30, 2023

CLA assistant check
All committers have signed the CLA.

@Gua00va Gua00va changed the title Added changes to handle default slasher broadcast. Yet to write tests Added changes to handle default slasher broadcast. May 30, 2023
@michaelsproul michaelsproul added the work-in-progress PR is a work-in-progress label May 30, 2023
@Gua00va
Copy link
Contributor Author

Gua00va commented May 31, 2023

@michaelsproul why is this local-testnet check not passing and how do I fix it?

@michaelsproul
Copy link
Member

@Gua00va I think it's just a spurious failure. Exit code 132 means illegal instruction: #3089

@Gua00va Gua00va closed this May 31, 2023
@Gua00va Gua00va reopened this May 31, 2023
@Gua00va
Copy link
Contributor Author

Gua00va commented May 31, 2023

Everything should work now I think!!

@Gua00va
Copy link
Contributor Author

Gua00va commented May 31, 2023

What is going wrong with the tests here? Can't understand. @michaelsproul

@michaelsproul
Copy link
Member

Looks like the release tests are failing because we have a default value for slasher-broadcast, but slasher-broadcast has a .require() on slasher. So it errors. We might need to remove the .require().

Try running the tests locally with cargo test --release -p lighthouse. Those tests are the same ones we should add to as well:

#[test]
fn slasher_broadcast_flag() {
CommandLineTest::new()
.flag("slasher", None)
.flag("slasher-broadcast", None)
.run_with_zero_port()
.with_config(|config| {
let slasher_config = config
.slasher
.as_ref()
.expect("Unable to parse Slasher config");
assert!(slasher_config.broadcast);
});
}

@Gua00va
Copy link
Contributor Author

Gua00va commented Jun 1, 2023

Looks like the release tests are failing because we have a default value for slasher-broadcast, but slasher-broadcast has a .require() on slasher. So it errors. We might need to remove the .require().

Try running the tests locally with cargo test --release -p lighthouse. Those tests are the same ones we should add to as well:

#[test]
fn slasher_broadcast_flag() {
CommandLineTest::new()
.flag("slasher", None)
.flag("slasher-broadcast", None)
.run_with_zero_port()
.with_config(|config| {
let slasher_config = config
.slasher
.as_ref()
.expect("Unable to parse Slasher config");
assert!(slasher_config.broadcast);
});
}

I am unable to run these tests because of mdbx compile error.

@michaelsproul
Copy link
Member

@Gua00va Ah, can you try rebasing on my branch? #4360

We're hopefully turning off MDBX by default.

@Gua00va
Copy link
Contributor Author

Gua00va commented Jun 2, 2023

@Gua00va Ah, can you try rebasing on my branch? #4360

We're hopefully turning off MDBX by default.

Which branch should I rebase on exactly??

@michaelsproul
Copy link
Member

@Gua00va the lmdb-default branch from my fork (michaelsproul/lighthouse).

@Gua00va Gua00va closed this Jun 2, 2023
bors bot pushed a commit that referenced this pull request Jun 8, 2023
## Issue Addressed

This PR addresses issue #4350

## Proposed Changes

This change will enable slasher broadcast in the following cases:
No flag is passed,
`--slasher-broadcast` is passed and,
`--slasher-broadcast=true` is passed.

Only when an explicit false value is passed the slasher does not broadcast.(`--slasher-broadcast=false`).

## Additional Info

TODO

- [x] Modify CLI parsing logic
- [x] Write test

Refer to #4353 


Co-authored-by: Rahul Dogra <rahulcooldogra@gmail.com>
Co-authored-by: Gua00va <105484243+Gua00va@users.noreply.github.com>
bors bot pushed a commit that referenced this pull request Jun 8, 2023
## Issue Addressed

This PR addresses issue #4350

## Proposed Changes

This change will enable slasher broadcast in the following cases:
No flag is passed,
`--slasher-broadcast` is passed and,
`--slasher-broadcast=true` is passed.

Only when an explicit false value is passed the slasher does not broadcast.(`--slasher-broadcast=false`).

## Additional Info

TODO

- [x] Modify CLI parsing logic
- [x] Write test

Refer to #4353 


Co-authored-by: Rahul Dogra <rahulcooldogra@gmail.com>
Co-authored-by: Gua00va <105484243+Gua00va@users.noreply.github.com>
bors bot pushed a commit that referenced this pull request Jun 8, 2023
## Issue Addressed

This PR addresses issue #4350

## Proposed Changes

This change will enable slasher broadcast in the following cases:
No flag is passed,
`--slasher-broadcast` is passed and,
`--slasher-broadcast=true` is passed.

Only when an explicit false value is passed the slasher does not broadcast.(`--slasher-broadcast=false`).

## Additional Info

TODO

- [x] Modify CLI parsing logic
- [x] Write test

Refer to #4353 


Co-authored-by: Rahul Dogra <rahulcooldogra@gmail.com>
Co-authored-by: Gua00va <105484243+Gua00va@users.noreply.github.com>
ghost pushed a commit to oone-world/lighthouse that referenced this pull request Jul 13, 2023
## Issue Addressed

This PR addresses issue sigp#4350

## Proposed Changes

This change will enable slasher broadcast in the following cases:
No flag is passed,
`--slasher-broadcast` is passed and,
`--slasher-broadcast=true` is passed.

Only when an explicit false value is passed the slasher does not broadcast.(`--slasher-broadcast=false`).

## Additional Info

TODO

- [x] Modify CLI parsing logic
- [x] Write test

Refer to sigp#4353 


Co-authored-by: Rahul Dogra <rahulcooldogra@gmail.com>
Co-authored-by: Gua00va <105484243+Gua00va@users.noreply.github.com>
Woodpile37 pushed a commit to Woodpile37/lighthouse that referenced this pull request Jan 6, 2024
## Issue Addressed

This PR addresses issue sigp#4350

## Proposed Changes

This change will enable slasher broadcast in the following cases:
No flag is passed,
`--slasher-broadcast` is passed and,
`--slasher-broadcast=true` is passed.

Only when an explicit false value is passed the slasher does not broadcast.(`--slasher-broadcast=false`).

## Additional Info

TODO

- [x] Modify CLI parsing logic
- [x] Write test

Refer to sigp#4353 


Co-authored-by: Rahul Dogra <rahulcooldogra@gmail.com>
Co-authored-by: Gua00va <105484243+Gua00va@users.noreply.github.com>
Woodpile37 pushed a commit to Woodpile37/lighthouse that referenced this pull request Jan 6, 2024
## Issue Addressed

This PR addresses issue sigp#4350

## Proposed Changes

This change will enable slasher broadcast in the following cases:
No flag is passed,
`--slasher-broadcast` is passed and,
`--slasher-broadcast=true` is passed.

Only when an explicit false value is passed the slasher does not broadcast.(`--slasher-broadcast=false`).

## Additional Info

TODO

- [x] Modify CLI parsing logic
- [x] Write test

Refer to sigp#4353 


Co-authored-by: Rahul Dogra <rahulcooldogra@gmail.com>
Co-authored-by: Gua00va <105484243+Gua00va@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
work-in-progress PR is a work-in-progress
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants