Update rust-bitcoin and add secp256k1 context randomization - #802

Merged
TheBlueMatt merged 7 commits into
lightningdevkit:mainfrom
TheBlueMatt:2021-01-update-rust-bitcoin
Mar 1, 2021
Merged

Update rust-bitcoin and add secp256k1 context randomization#802
TheBlueMatt merged 7 commits into
lightningdevkit:mainfrom
TheBlueMatt:2021-01-update-rust-bitcoin

Conversation

@TheBlueMatt

@TheBlueMattTheBlueMatt commented Feb 19, 2021•
edited
Loading

Copy link
Copy Markdown
Collaborator

This updates our rust-bitcoin dependency and adds secp256k1 context randomization. Note that it relies on an open PR upstream in lightning-c-bindings and a separate open PR upstream for fuzzing. That's probably fine for fuzzing, but for bindings it kinda sucks. Hopefully we can get those merged soon.

@TheBlueMattTheBlueMatt changed the title Update rust-bitcoinUpdate rust-bitcoin and add secp256k1 context randomizationFeb 19, 2021
@TheBlueMatt

TheBlueMatt commented Feb 19, 2021

Copy link
Copy Markdown
CollaboratorAuthor

Also added a wasm32-wasi bindings build for good measure (since we need the update to do it).

@TheBlueMatt
TheBlueMattforce-pushed the 2021-01-update-rust-bitcoin branch 3 times, most recently from 5a166a1 to e81fab9CompareFebruary 19, 2021 19:41
@codecov

codecovBot commented Feb 19, 2021•
edited
Loading

Copy link
Copy Markdown

Codecov Report

Merging #802 (ea48a5a) into main (e77b160) will decrease coverage by 0.01%.
The diff coverage is 96.00%.

Impacted file tree graph

@@ Coverage Diff @@## main #802 +/- ##
==========================================
- Coverage 91.04% 91.02% -0.02% 
==========================================
Files 48 48 Lines 25480 25492 +12 ==========================================
+ Hits 23199 23205 +6 - Misses 2281 2287 +6
Impacted FilesCoverage Δ
lightning/src/util/ser.rs90.90% <0.00%> (+0.29%)⬆️
lightning/src/chain/keysinterface.rs93.15% <90.00%> (-0.21%)⬇️
lightning/src/chain/channelmonitor.rs95.76% <100.00%> (+0.08%)⬆️
lightning/src/ln/channel.rs87.83% <100.00%> (+0.03%)⬆️
lightning/src/ln/channelmanager.rs85.26% <100.00%> (+0.02%)⬆️
lightning/src/ln/onchaintx.rs94.31% <100.00%> (+0.20%)⬆️
lightning/src/ln/onion_route_tests.rs96.85% <100.00%> (ø)
lightning/src/util/test_utils.rs83.49% <100.00%> (+0.33%)⬆️
lightning/src/ln/functional_tests.rs96.97% <0.00%> (-0.16%)⬇️

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 e77b160...ea48a5a. Read the comment docs.

@TheBlueMatt
TheBlueMattforce-pushed the 2021-01-update-rust-bitcoin branch 3 times, most recently from a5cb963 to 85dcb52CompareFebruary 19, 2021 21:12
valentinewallace
valentinewallace previously approved these changes Feb 19, 2021
View reviewed changes

@valentinewallacevalentinewallace left a comment •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for taking care of this! Nice bite-sized commits + good to squeeze in the side channel attack mitigation.

Edit: fine to approve once CI is fixed

Comment threadlightning/src/util/test_utils.rs Outdated
Comment threadlightning/src/ln/onion_route_tests.rs
use bitcoin::secp256k1::Signature;
msgs::ChannelUpdate {
signature: Signature::from(FFISignature::new()),
signature: Signature::from(unsafe { FFISignature::new() }),

@valentinewallacevalentinewallaceFeb 19, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

OOC, do you know why secp256k1:;Signature was suddenly made unsafe?

@TheBlueMattTheBlueMattFeb 19, 2021

Copy link
Copy Markdown
CollaboratorAuthor

Choose a reason for hiding this comment

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

I'm not 100% sure, but it does allow you to create an obviously-bogus Signature object which generally you're not supposed to be able to do (once its in a Signature its supposed to be a valid object, even if not a valid signature for a given message, I think).

Comment threadgenbindings.sh Outdated
@TheBlueMatt

TheBlueMatt commented Feb 19, 2021

Copy link
Copy Markdown
CollaboratorAuthor

Yea, sorry, I got too excited with this. I think I also need to pull in rust-bitcoin/bitcoin_hashes#111 for fuzzing and then re-redo the full_stack_target test.

@TheBlueMatt
TheBlueMattforce-pushed the 2021-01-update-rust-bitcoin branch from 85dcb52 to 3dd72e5CompareFebruary 19, 2021 22:49
@TheBlueMatt

TheBlueMatt commented Feb 19, 2021

Copy link
Copy Markdown
CollaboratorAuthor

OK, should be resolve now, also address your comments.

@TheBlueMatt
TheBlueMattforce-pushed the 2021-01-update-rust-bitcoin branch 2 times, most recently from 386ccf3 to 49718cfCompareFebruary 19, 2021 23:11
valentinewallace
valentinewallace reviewed Feb 20, 2021
View reviewed changes
Comment threadc-bindings-gen/README.md Outdated
@TheBlueMatt
TheBlueMattforce-pushed the 2021-01-update-rust-bitcoin branch from 49718cf to a7863d4CompareFebruary 22, 2021 17:00
@TheBlueMatt

TheBlueMatt commented Feb 22, 2021

Copy link
Copy Markdown
CollaboratorAuthor

Rebased.

jkczyz
jkczyz reviewed Feb 23, 2021
View reviewed changes

@jkczyzjkczyz left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Given the dependencies on open PRs, should we wait at least on some concept ACKs on those? I'm not sure if I have suitable knowledge to do so on my own.

Comment threadlightning/src/util/test_utils.rs Outdated
Comment threadfuzz/src/full_stack.rs
// Writing new code generating transactions and see a new failure ? Don't forget to add input for the FuzzEstimator !

// 0000000000000000000000000000000000000000000000000000000000000000 - our network key
// 0100000000000000000000000000000000000000000000000000000000000000 - our network key

@jkczyzjkczyzFeb 23, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

To confirm my understanding, do these diffs extend from the fact that the use of get_secure_random_bytes to seed Secp256k1 causes an increment to a rand_bytes_child_index?

@TheBlueMattTheBlueMattFeb 24, 2021

Copy link
Copy Markdown
CollaboratorAuthor

Choose a reason for hiding this comment

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

They extend from a few things (which is why there's so many of them). This particular one extends from the fact that secp256k1's fuzzing mode now requires the private key not be all-0s (in fact it enforces the regular non-fuzzing rules around private key validity). Other changes are required because, as a result of that, we now have sha256 only return hashes with one byte set to 1-0xff instead of 0-0xff as we did previously.

Comment threadgenbindings.sh Outdated
@TheBlueMatt
TheBlueMattforce-pushed the 2021-01-update-rust-bitcoin branch from a7863d4 to 4e2bdf2CompareFebruary 24, 2021 04:15
@TheBlueMatt

TheBlueMatt commented Feb 24, 2021

Copy link
Copy Markdown
CollaboratorAuthor

Given the dependencies on open PRs, should we wait at least on some concept ACKs on those?

Yea, to merge that probably makes sense.

@TheBlueMatt

TheBlueMatt commented Feb 26, 2021

Copy link
Copy Markdown
CollaboratorAuthor

Relevant IRC discussion:

<BlueMatt> andytoshi: plz2conceptack https://github.com/rust-bitcoin/rust-secp256k1/pull/282
...
<andytoshi> BlueMatt: concept ack
<BlueMatt> ariard: I should probably listen in to those, but not sure I have much to add.
<BlueMatt> andytoshi: note there's a bitcoin_hashes companion which checks for 0 hashes and returns 1 instead
<andytoshi> nice, concept ack that too
<BlueMatt> ok, thanks
<BlueMatt> I'm gonna depend on those PRs in rust-lightning's fuzzing subcrate, fwiw
<BlueMatt> and also depend on https://github.com/rust-bitcoin/rust-secp256k1/pull/279 in our c bindings
<BlueMatt> (though *most* of our contexts are randomized, thats just for one group of them, which honestly I should probably just drop in the api)
<andytoshi> yeah concept ack that too
<BlueMatt> oh, no, I cant drop all of them, damn.
<andytoshi> i'm not so worried about the context randomization, it's a defense in depth against sidechannel attacks and i'd be hard-pressed to demonstrate a benefit
<andytoshi> not that you shouldn't do it ... but it's far from critical
<BlueMatt> yea....I'd need to audit, but it may very well be the only places we actually use the global contexts are verify tables anyway
<BlueMatt> oh, no, we do public key combines and private key multiplies and such which takes a signing context

@TheBlueMatt
TheBlueMattforce-pushed the 2021-01-update-rust-bitcoin branch 2 times, most recently from 1543730 to b328fcfCompareFebruary 26, 2021 16:32
@TheBlueMatt

TheBlueMatt commented Feb 26, 2021

Copy link
Copy Markdown
CollaboratorAuthor

Squashed. I'd ideally like to take this for 0.0.13, unless someone has objections to the PR dependencies, which would be reasonable.

@TheBlueMattTheBlueMatt added this to the 0.0.13 milestone Feb 26, 2021
@valentinewallace

valentinewallace commented Feb 26, 2021

Copy link
Copy Markdown
Contributor

fix build?

@TheBlueMatt
Update rust-bitcoin
a51d5ce
@TheBlueMatt
Apply --cfg=fuzzing when fuzzing, as required by new secp256k1
9b7609a
Note that rust-fuzz wrappers (including honggfuzz) already apply
this for us.
@TheBlueMatt
TheBlueMattforce-pushed the 2021-01-update-rust-bitcoin branch from b328fcf to bd48726CompareFebruary 26, 2021 20:28
@TheBlueMatt

TheBlueMatt commented Feb 26, 2021

Copy link
Copy Markdown
CollaboratorAuthor

Oops, rebase introduced errors. I droped the bindings updates cause its gonna require some minor changes in the generator, though likely not too bad.

valentinewallace
valentinewallace approved these changes Feb 26, 2021
View reviewed changes

@valentinewallacevalentinewallace left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM!

@TheBlueMattTheBlueMatt mentioned this pull request Feb 26, 2021
Merged
@TheBlueMatt
TheBlueMattforce-pushed the 2021-01-update-rust-bitcoin branch from bd48726 to ea48a5aCompareFebruary 27, 2021 04:50
@TheBlueMatt

TheBlueMatt commented Feb 27, 2021

Copy link
Copy Markdown
CollaboratorAuthor

Bumped to latest version of the rust-secp pull to fix (most of) the performance degredation.

@TheBlueMatt

TheBlueMatt commented Feb 27, 2021

Copy link
Copy Markdown
CollaboratorAuthor

Note that, sadly, this still decreases the performance of, especially, chanmon_consistency, taking the CI fuzz runs from 18/91 minutes to 22/23 minutes for the fuzzing section.

@jkczyz

jkczyz commented Feb 28, 2021

Copy link
Copy Markdown
Contributor

I'm seeing this failure at ea48a5a:

$ cargo test
Finished test [optimized + debuginfo] target(s) in 0.04s
Running target/debug/deps/lightning_fuzz-2b6f767ab0ec4198
running 1 test
test full_stack::tests::test_no_existing_test_breakage ... FAILED
failures:
---- full_stack::tests::test_no_existing_test_breakage stdout ----
TRACE [lightning::ln::peer_handler : /Users/jkczyz/src/rust-lightning-review/lightning/src/ln/peer_handler.rs, 579] Got Err handling message, disconnecting peer because Bad MAC
thread 'full_stack::tests::test_no_existing_test_breakage' panicked at 'assertion failed: `(left == right)`
left: `None`,
right: `Some(1)`', src/full_stack.rs:902:9
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
failures:
full_stack::tests::test_no_existing_test_breakage
test result: FAILED. 0 passed; 1 failed; 0 ignored; 0 measured; 0 filtered out
error: test failed, to rerun pass '--lib'

@TheBlueMatt

TheBlueMatt commented Feb 28, 2021•
edited
Loading

Copy link
Copy Markdown
CollaboratorAuthor

Right, you now need to set RUSTFLAGS="--cfg fuzzing" in order to run the fuzzing tests.

jkczyz
jkczyz approved these changes Feb 28, 2021
View reviewed changes

@jkczyzjkczyz left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Right, you now need to set RUSTFLAGS="--cfg fuzzing" in order to run the fuzzing tests.

Yeah, that should have been obvious. :P

LGTM

@TheBlueMatt
TheBlueMatt merged commit 9fba7c9 into lightningdevkit:mainMar 1, 2021
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jkczyzjkczyzjkczyz approved these changes
@valentinewallacevalentinewallacevalentinewallace approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

0.0.13

Development

Successfully merging this pull request may close these issues.

3 participants

@TheBlueMatt@valentinewallace@jkczyz
, 'i'); if (__m === '*' || __re.test(location.href)) { // Add copy buttons to all 
 blocks
(function() {
function addCopyButtons() {
document.querySelectorAll('pre code').forEach(function(codeBlock) {
if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;
codeBlock.parentElement.setAttribute('data-copy-added', 'true');
var btn = document.createElement('button');
btn.textContent = 'Copy';
btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';
btn.onmouseover = function() { this.style.opacity = '1'; };
btn.onmouseout = function() { this.style.opacity = '0.7'; };
btn.onclick = function() {
navigator.clipboard.writeText(codeBlock.textContent).then(function() {
btn.textContent = 'Copied!';
setTimeout(function() { btn.textContent = 'Copy'; }, 1500);
});
};
codeBlock.parentElement.style.position = 'relative';
codeBlock.parentElement.appendChild(btn);
});
}
addCopyButtons();
// Re-run on dynamic content
var observer = new MutationObserver(addCopyButtons);
observer.observe(document.body, { childList: true, subtree: true });
})();
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '

Skip to content
Update rust-bitcoin and add secp256k1 context randomization - #802
Merged
TheBlueMatt merged 7 commits into
lightningdevkit:mainfrom 
TheBlueMatt:2021-01-update-rust-bitcoin
Mar 1, 2021
Merged
Update rust-bitcoin and add secp256k1 context randomization#802
TheBlueMatt merged 7 commits into
lightningdevkit:mainfrom 
TheBlueMatt:2021-01-update-rust-bitcoin
Conversation
@TheBlueMatt
@TheBlueMattTheBlueMatt
commented
Feb 19, 2021•

edited

Loading

Copy link


Copy Markdown

Collaborator
This updates our rust-bitcoin dependency and adds secp256k1 context randomization. Note that it relies on an open PR upstream in lightning-c-bindings and a separate open PR upstream for fuzzing. That's probably fine for fuzzing, but for bindings it kinda sucks. Hopefully we can get those merged soon.
@TheBlueMattTheBlueMatt
changed the title
Update rust-bitcoinUpdate rust-bitcoin and add secp256k1 context randomizationFeb 19, 2021
@TheBlueMatt
TheBlueMatt
commented
Feb 19, 2021

Copy link


Copy Markdown

CollaboratorAuthor
Also added a wasm32-wasi bindings build for good measure (since we need the update to do it).
@TheBlueMatt
TheBlueMattforce-pushed
the
2021-01-update-rust-bitcoin
branch
3 times, most recently
from
5a166a1 to
e81fab9CompareFebruary 19, 2021 19:41
@codecov
codecovBot
commented
Feb 19, 2021•

edited

Loading

Copy link


Copy Markdown

Codecov Report
Merging #802 (ea48a5a) into main (e77b160) will decrease coverage by 0.01%.

The diff coverage is 96.00%.
Impacted file tree graph
@@ Coverage Diff @@## main #802 +/- ##
==========================================
- Coverage 91.04% 91.02% -0.02% 
==========================================
Files 48 48 Lines 25480 25492 +12 ==========================================
+ Hits 23199 23205 +6 - Misses 2281 2287 +6 
Impacted FilesCoverage Δ
lightning/src/util/ser.rs90.90% <0.00%> (+0.29%)⬆️
lightning/src/chain/keysinterface.rs93.15% <90.00%> (-0.21%)⬇️
lightning/src/chain/channelmonitor.rs95.76% <100.00%> (+0.08%)⬆️
lightning/src/ln/channel.rs87.83% <100.00%> (+0.03%)⬆️
lightning/src/ln/channelmanager.rs85.26% <100.00%> (+0.02%)⬆️
lightning/src/ln/onchaintx.rs94.31% <100.00%> (+0.20%)⬆️
lightning/src/ln/onion_route_tests.rs96.85% <100.00%> (ø)
lightning/src/util/test_utils.rs83.49% <100.00%> (+0.33%)⬆️
lightning/src/ln/functional_tests.rs96.97% <0.00%> (-0.16%)⬇️
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 e77b160...ea48a5a. Read the comment docs.
@TheBlueMatt
TheBlueMattforce-pushed
the
2021-01-update-rust-bitcoin
branch
3 times, most recently
from
a5cb963 to
85dcb52CompareFebruary 19, 2021 21:12
valentinewallace
valentinewallace
previously approved these changes
Feb 19, 2021
View reviewed changes
@valentinewallacevalentinewallace
left a comment
•

edited

Loading

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Thanks for taking care of this! Nice bite-sized commits + good to squeeze in the side channel attack mitigation.
Edit: fine to approve once CI is fixed
Comment threadlightning/src/util/test_utils.rs
Outdated

Comment threadlightning/src/ln/onion_route_tests.rs
 use bitcoin::secp256k1::Signature;
 msgs::ChannelUpdate {
 signature: Signature::from(FFISignature::new()),
 signature: Signature::from(unsafe { FFISignature::new() }),
@valentinewallacevalentinewallaceFeb 19, 2021

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

OOC, do you know why secp256k1:;Signature was suddenly made unsafe?
@TheBlueMattTheBlueMattFeb 19, 2021

Copy link


Copy Markdown

CollaboratorAuthor
Choose a reason for hiding this comment

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

I'm not 100% sure, but it does allow you to create an obviously-bogus Signature object which generally you're not supposed to be able to do (once its in a Signature its supposed to be a valid object, even if not a valid signature for a given message, I think).
Comment threadgenbindings.sh
Outdated

@TheBlueMatt
TheBlueMatt
commented
Feb 19, 2021

Copy link


Copy Markdown

CollaboratorAuthor
Yea, sorry, I got too excited with this. I think I also need to pull in rust-bitcoin/bitcoin_hashes#111 for fuzzing and then re-redo the full_stack_target test.
@TheBlueMatt
TheBlueMattforce-pushed
the
2021-01-update-rust-bitcoin
branch
from
85dcb52 to
3dd72e5CompareFebruary 19, 2021 22:49
@TheBlueMatt
TheBlueMatt
commented
Feb 19, 2021

Copy link


Copy Markdown

CollaboratorAuthor
OK, should be resolve now, also address your comments.
@TheBlueMatt
TheBlueMattforce-pushed
the
2021-01-update-rust-bitcoin
branch
2 times, most recently
from
386ccf3 to
49718cfCompareFebruary 19, 2021 23:11
valentinewallace
valentinewallace
reviewed
Feb 20, 2021
View reviewed changes
Comment threadc-bindings-gen/README.md
Outdated

@TheBlueMatt
TheBlueMattforce-pushed
the
2021-01-update-rust-bitcoin
branch
from
49718cf to
a7863d4CompareFebruary 22, 2021 17:00
@TheBlueMatt
TheBlueMatt
commented
Feb 22, 2021

Copy link


Copy Markdown

CollaboratorAuthor
Rebased.
jkczyz
jkczyz
reviewed
Feb 23, 2021
View reviewed changes
@jkczyzjkczyz
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Given the dependencies on open PRs, should we wait at least on some concept ACKs on those? I'm not sure if I have suitable knowledge to do so on my own.
Comment threadlightning/src/util/test_utils.rs
Outdated

Comment threadfuzz/src/full_stack.rs
 // Writing new code generating transactions and see a new failure ? Don't forget to add input for the FuzzEstimator !

 // 0000000000000000000000000000000000000000000000000000000000000000 - our network key
 // 0100000000000000000000000000000000000000000000000000000000000000 - our network key
@jkczyzjkczyzFeb 23, 2021

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

To confirm my understanding, do these diffs extend from the fact that the use of get_secure_random_bytes to seed Secp256k1 causes an increment to a rand_bytes_child_index?
@TheBlueMattTheBlueMattFeb 24, 2021

Copy link


Copy Markdown

CollaboratorAuthor
Choose a reason for hiding this comment

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

They extend from a few things (which is why there's so many of them). This particular one extends from the fact that secp256k1's fuzzing mode now requires the private key not be all-0s (in fact it enforces the regular non-fuzzing rules around private key validity). Other changes are required because, as a result of that, we now have sha256 only return hashes with one byte set to 1-0xff instead of 0-0xff as we did previously.
Comment threadgenbindings.sh
Outdated

@TheBlueMatt
TheBlueMattforce-pushed
the
2021-01-update-rust-bitcoin
branch
from
a7863d4 to
4e2bdf2CompareFebruary 24, 2021 04:15
@TheBlueMatt
TheBlueMatt
commented
Feb 24, 2021

Copy link


Copy Markdown

CollaboratorAuthor
Given the dependencies on open PRs, should we wait at least on some concept ACKs on those?
Yea, to merge that probably makes sense.
@TheBlueMatt
TheBlueMatt
commented
Feb 26, 2021

Copy link


Copy Markdown

CollaboratorAuthor
Relevant IRC discussion:
<BlueMatt> andytoshi: plz2conceptack https://github.com/rust-bitcoin/rust-secp256k1/pull/282
...
<andytoshi> BlueMatt: concept ack
<BlueMatt> ariard: I should probably listen in to those, but not sure I have much to add.
<BlueMatt> andytoshi: note there's a bitcoin_hashes companion which checks for 0 hashes and returns 1 instead
<andytoshi> nice, concept ack that too
<BlueMatt> ok, thanks
<BlueMatt> I'm gonna depend on those PRs in rust-lightning's fuzzing subcrate, fwiw
<BlueMatt> and also depend on https://github.com/rust-bitcoin/rust-secp256k1/pull/279 in our c bindings
<BlueMatt> (though *most* of our contexts are randomized, thats just for one group of them, which honestly I should probably just drop in the api)
<andytoshi> yeah concept ack that too
<BlueMatt> oh, no, I cant drop all of them, damn.
<andytoshi> i'm not so worried about the context randomization, it's a defense in depth against sidechannel attacks and i'd be hard-pressed to demonstrate a benefit
<andytoshi> not that you shouldn't do it ... but it's far from critical
<BlueMatt> yea....I'd need to audit, but it may very well be the only places we actually use the global contexts are verify tables anyway
<BlueMatt> oh, no, we do public key combines and private key multiplies and such which takes a signing context

@TheBlueMatt
TheBlueMattforce-pushed
the
2021-01-update-rust-bitcoin
branch
2 times, most recently
from
1543730 to
b328fcfCompareFebruary 26, 2021 16:32
@TheBlueMatt
TheBlueMatt
commented
Feb 26, 2021

Copy link


Copy Markdown

CollaboratorAuthor
Squashed. I'd ideally like to take this for 0.0.13, unless someone has objections to the PR dependencies, which would be reasonable.
@TheBlueMattTheBlueMatt
added this to the 0.0.13 milestone
Feb 26, 2021
@valentinewallace
valentinewallace
commented
Feb 26, 2021

Copy link


Copy Markdown

Contributor
fix build?
@TheBlueMatt
Update rust-bitcoin
a51d5ce
@TheBlueMatt
Apply --cfg=fuzzing when fuzzing, as required by new secp256k1
9b7609a
Note that rust-fuzz wrappers (including honggfuzz) already apply
this for us.
@TheBlueMatt
TheBlueMattforce-pushed
the
2021-01-update-rust-bitcoin
branch
from
b328fcf to
bd48726CompareFebruary 26, 2021 20:28
@TheBlueMatt
TheBlueMatt
commented
Feb 26, 2021

Copy link


Copy Markdown

CollaboratorAuthor
Oops, rebase introduced errors. I droped the bindings updates cause its gonna require some minor changes in the generator, though likely not too bad.
valentinewallace
valentinewallace
approved these changes
Feb 26, 2021
View reviewed changes
@valentinewallacevalentinewallace
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

LGTM!
@TheBlueMattTheBlueMatt
mentioned this pull request
Feb 26, 2021
 Merged

@TheBlueMatt
TheBlueMattforce-pushed
the
2021-01-update-rust-bitcoin
branch
from
bd48726 to
ea48a5aCompareFebruary 27, 2021 04:50
@TheBlueMatt
TheBlueMatt
commented
Feb 27, 2021

Copy link


Copy Markdown

CollaboratorAuthor
Bumped to latest version of the rust-secp pull to fix (most of) the performance degredation.
@TheBlueMatt
TheBlueMatt
commented
Feb 27, 2021

Copy link


Copy Markdown

CollaboratorAuthor
Note that, sadly, this still decreases the performance of, especially, chanmon_consistency, taking the CI fuzz runs from 18/91 minutes to 22/23 minutes for the fuzzing section.
@jkczyz
jkczyz
commented
Feb 28, 2021

Copy link


Copy Markdown

Contributor
I'm seeing this failure at ea48a5a:
$ cargo test
Finished test [optimized + debuginfo] target(s) in 0.04s
Running target/debug/deps/lightning_fuzz-2b6f767ab0ec4198
running 1 test
test full_stack::tests::test_no_existing_test_breakage ... FAILED
failures:
---- full_stack::tests::test_no_existing_test_breakage stdout ----
TRACE [lightning::ln::peer_handler : /Users/jkczyz/src/rust-lightning-review/lightning/src/ln/peer_handler.rs, 579] Got Err handling message, disconnecting peer because Bad MAC
thread 'full_stack::tests::test_no_existing_test_breakage' panicked at 'assertion failed: `(left == right)`
left: `None`,
right: `Some(1)`', src/full_stack.rs:902:9
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
failures:
full_stack::tests::test_no_existing_test_breakage
test result: FAILED. 0 passed; 1 failed; 0 ignored; 0 measured; 0 filtered out
error: test failed, to rerun pass '--lib'

@TheBlueMatt
TheBlueMatt
commented
Feb 28, 2021•

edited

Loading

Copy link


Copy Markdown

CollaboratorAuthor
Right, you now need to set RUSTFLAGS="--cfg fuzzing" in order to run the fuzzing tests.
jkczyz
jkczyz
approved these changes
Feb 28, 2021
View reviewed changes
@jkczyzjkczyz
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Right, you now need to set RUSTFLAGS="--cfg fuzzing" in order to run the fuzzing tests.
Yeah, that should have been obvious. :P
LGTM
@TheBlueMatt
TheBlueMatt
merged commit 9fba7c9
into
lightningdevkit:mainMar 1, 2021
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@jkczyzjkczyzjkczyz approved these changes
@valentinewallacevalentinewallacevalentinewallace approved these changes

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone

0.0.13

Development

Successfully merging this pull request may close these issues.

3 participants

@TheBlueMatt@valentinewallace@jkczyz

, 'i');
if (__m === '*' || __re.test(location.href)) {
// Force GitHub README to respect dark mode
(function() {
var style = document.createElement('style');
style.textContent = '
.markdown-body {
color-scheme: dark light;
}
.markdown-body pre { background: #161b22 !important; }
.markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }
.markdown-body table th, .markdown-body table td { border-color: #30363d !important; }
.markdown-body img { background: #0d1117; }
.markdown-body blockquote { border-left-color: #8b949e; }
.markdown-body hr { border-color: #30363d; }
';
document.head.appendChild(style);
})();
}
} catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
Update rust-bitcoin and add secp256k1 context randomization - #802
Merged
TheBlueMatt merged 7 commits into
lightningdevkit:mainfrom 
TheBlueMatt:2021-01-update-rust-bitcoin
Mar 1, 2021
Merged
Update rust-bitcoin and add secp256k1 context randomization#802
TheBlueMatt merged 7 commits into
lightningdevkit:mainfrom 
TheBlueMatt:2021-01-update-rust-bitcoin
Conversation
@TheBlueMatt
@TheBlueMattTheBlueMatt
commented
Feb 19, 2021•

edited

Loading

Copy link


Copy Markdown

Collaborator
This updates our rust-bitcoin dependency and adds secp256k1 context randomization. Note that it relies on an open PR upstream in lightning-c-bindings and a separate open PR upstream for fuzzing. That's probably fine for fuzzing, but for bindings it kinda sucks. Hopefully we can get those merged soon.
@TheBlueMattTheBlueMatt
changed the title
Update rust-bitcoinUpdate rust-bitcoin and add secp256k1 context randomizationFeb 19, 2021
@TheBlueMatt
TheBlueMatt
commented
Feb 19, 2021

Copy link


Copy Markdown

CollaboratorAuthor
Also added a wasm32-wasi bindings build for good measure (since we need the update to do it).
@TheBlueMatt
TheBlueMattforce-pushed
the
2021-01-update-rust-bitcoin
branch
3 times, most recently
from
5a166a1 to
e81fab9CompareFebruary 19, 2021 19:41
@codecov
codecovBot
commented
Feb 19, 2021•

edited

Loading

Copy link


Copy Markdown

Codecov Report
Merging #802 (ea48a5a) into main (e77b160) will decrease coverage by 0.01%.

The diff coverage is 96.00%.
Impacted file tree graph
@@ Coverage Diff @@## main #802 +/- ##
==========================================
- Coverage 91.04% 91.02% -0.02% 
==========================================
Files 48 48 Lines 25480 25492 +12 ==========================================
+ Hits 23199 23205 +6 - Misses 2281 2287 +6 
Impacted FilesCoverage Δ
lightning/src/util/ser.rs90.90% <0.00%> (+0.29%)⬆️
lightning/src/chain/keysinterface.rs93.15% <90.00%> (-0.21%)⬇️
lightning/src/chain/channelmonitor.rs95.76% <100.00%> (+0.08%)⬆️
lightning/src/ln/channel.rs87.83% <100.00%> (+0.03%)⬆️
lightning/src/ln/channelmanager.rs85.26% <100.00%> (+0.02%)⬆️
lightning/src/ln/onchaintx.rs94.31% <100.00%> (+0.20%)⬆️
lightning/src/ln/onion_route_tests.rs96.85% <100.00%> (ø)
lightning/src/util/test_utils.rs83.49% <100.00%> (+0.33%)⬆️
lightning/src/ln/functional_tests.rs96.97% <0.00%> (-0.16%)⬇️
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 e77b160...ea48a5a. Read the comment docs.
@TheBlueMatt
TheBlueMattforce-pushed
the
2021-01-update-rust-bitcoin
branch
3 times, most recently
from
a5cb963 to
85dcb52CompareFebruary 19, 2021 21:12
valentinewallace
valentinewallace
previously approved these changes
Feb 19, 2021
View reviewed changes
@valentinewallacevalentinewallace
left a comment
•

edited

Loading

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Thanks for taking care of this! Nice bite-sized commits + good to squeeze in the side channel attack mitigation.
Edit: fine to approve once CI is fixed
Comment threadlightning/src/util/test_utils.rs
Outdated

Comment threadlightning/src/ln/onion_route_tests.rs
 use bitcoin::secp256k1::Signature;
 msgs::ChannelUpdate {
 signature: Signature::from(FFISignature::new()),
 signature: Signature::from(unsafe { FFISignature::new() }),
@valentinewallacevalentinewallaceFeb 19, 2021

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

OOC, do you know why secp256k1:;Signature was suddenly made unsafe?
@TheBlueMattTheBlueMattFeb 19, 2021

Copy link


Copy Markdown

CollaboratorAuthor
Choose a reason for hiding this comment

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

I'm not 100% sure, but it does allow you to create an obviously-bogus Signature object which generally you're not supposed to be able to do (once its in a Signature its supposed to be a valid object, even if not a valid signature for a given message, I think).
Comment threadgenbindings.sh
Outdated

@TheBlueMatt
TheBlueMatt
commented
Feb 19, 2021

Copy link


Copy Markdown

CollaboratorAuthor
Yea, sorry, I got too excited with this. I think I also need to pull in rust-bitcoin/bitcoin_hashes#111 for fuzzing and then re-redo the full_stack_target test.
@TheBlueMatt
TheBlueMattforce-pushed
the
2021-01-update-rust-bitcoin
branch
from
85dcb52 to
3dd72e5CompareFebruary 19, 2021 22:49
@TheBlueMatt
TheBlueMatt
commented
Feb 19, 2021

Copy link


Copy Markdown

CollaboratorAuthor
OK, should be resolve now, also address your comments.
@TheBlueMatt
TheBlueMattforce-pushed
the
2021-01-update-rust-bitcoin
branch
2 times, most recently
from
386ccf3 to
49718cfCompareFebruary 19, 2021 23:11
valentinewallace
valentinewallace
reviewed
Feb 20, 2021
View reviewed changes
Comment threadc-bindings-gen/README.md
Outdated

@TheBlueMatt
TheBlueMattforce-pushed
the
2021-01-update-rust-bitcoin
branch
from
49718cf to
a7863d4CompareFebruary 22, 2021 17:00
@TheBlueMatt
TheBlueMatt
commented
Feb 22, 2021

Copy link


Copy Markdown

CollaboratorAuthor
Rebased.
jkczyz
jkczyz
reviewed
Feb 23, 2021
View reviewed changes
@jkczyzjkczyz
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Given the dependencies on open PRs, should we wait at least on some concept ACKs on those? I'm not sure if I have suitable knowledge to do so on my own.
Comment threadlightning/src/util/test_utils.rs
Outdated

Comment threadfuzz/src/full_stack.rs
 // Writing new code generating transactions and see a new failure ? Don't forget to add input for the FuzzEstimator !

 // 0000000000000000000000000000000000000000000000000000000000000000 - our network key
 // 0100000000000000000000000000000000000000000000000000000000000000 - our network key
@jkczyzjkczyzFeb 23, 2021

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

To confirm my understanding, do these diffs extend from the fact that the use of get_secure_random_bytes to seed Secp256k1 causes an increment to a rand_bytes_child_index?
@TheBlueMattTheBlueMattFeb 24, 2021

Copy link


Copy Markdown

CollaboratorAuthor
Choose a reason for hiding this comment

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

They extend from a few things (which is why there's so many of them). This particular one extends from the fact that secp256k1's fuzzing mode now requires the private key not be all-0s (in fact it enforces the regular non-fuzzing rules around private key validity). Other changes are required because, as a result of that, we now have sha256 only return hashes with one byte set to 1-0xff instead of 0-0xff as we did previously.
Comment threadgenbindings.sh
Outdated

@TheBlueMatt
TheBlueMattforce-pushed
the
2021-01-update-rust-bitcoin
branch
from
a7863d4 to
4e2bdf2CompareFebruary 24, 2021 04:15
@TheBlueMatt
TheBlueMatt
commented
Feb 24, 2021

Copy link


Copy Markdown

CollaboratorAuthor
Given the dependencies on open PRs, should we wait at least on some concept ACKs on those?
Yea, to merge that probably makes sense.
@TheBlueMatt
TheBlueMatt
commented
Feb 26, 2021

Copy link


Copy Markdown

CollaboratorAuthor
Relevant IRC discussion:
<BlueMatt> andytoshi: plz2conceptack https://github.com/rust-bitcoin/rust-secp256k1/pull/282
...
<andytoshi> BlueMatt: concept ack
<BlueMatt> ariard: I should probably listen in to those, but not sure I have much to add.
<BlueMatt> andytoshi: note there's a bitcoin_hashes companion which checks for 0 hashes and returns 1 instead
<andytoshi> nice, concept ack that too
<BlueMatt> ok, thanks
<BlueMatt> I'm gonna depend on those PRs in rust-lightning's fuzzing subcrate, fwiw
<BlueMatt> and also depend on https://github.com/rust-bitcoin/rust-secp256k1/pull/279 in our c bindings
<BlueMatt> (though *most* of our contexts are randomized, thats just for one group of them, which honestly I should probably just drop in the api)
<andytoshi> yeah concept ack that too
<BlueMatt> oh, no, I cant drop all of them, damn.
<andytoshi> i'm not so worried about the context randomization, it's a defense in depth against sidechannel attacks and i'd be hard-pressed to demonstrate a benefit
<andytoshi> not that you shouldn't do it ... but it's far from critical
<BlueMatt> yea....I'd need to audit, but it may very well be the only places we actually use the global contexts are verify tables anyway
<BlueMatt> oh, no, we do public key combines and private key multiplies and such which takes a signing context

@TheBlueMatt
TheBlueMattforce-pushed
the
2021-01-update-rust-bitcoin
branch
2 times, most recently
from
1543730 to
b328fcfCompareFebruary 26, 2021 16:32
@TheBlueMatt
TheBlueMatt
commented
Feb 26, 2021

Copy link


Copy Markdown

CollaboratorAuthor
Squashed. I'd ideally like to take this for 0.0.13, unless someone has objections to the PR dependencies, which would be reasonable.
@TheBlueMattTheBlueMatt
added this to the 0.0.13 milestone
Feb 26, 2021
@valentinewallace
valentinewallace
commented
Feb 26, 2021

Copy link


Copy Markdown

Contributor
fix build?
@TheBlueMatt
Update rust-bitcoin
a51d5ce
@TheBlueMatt
Apply --cfg=fuzzing when fuzzing, as required by new secp256k1
9b7609a
Note that rust-fuzz wrappers (including honggfuzz) already apply
this for us.
@TheBlueMatt
TheBlueMattforce-pushed
the
2021-01-update-rust-bitcoin
branch
from
b328fcf to
bd48726CompareFebruary 26, 2021 20:28
@TheBlueMatt
TheBlueMatt
commented
Feb 26, 2021

Copy link


Copy Markdown

CollaboratorAuthor
Oops, rebase introduced errors. I droped the bindings updates cause its gonna require some minor changes in the generator, though likely not too bad.
valentinewallace
valentinewallace
approved these changes
Feb 26, 2021
View reviewed changes
@valentinewallacevalentinewallace
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

LGTM!
@TheBlueMattTheBlueMatt
mentioned this pull request
Feb 26, 2021
 Merged

@TheBlueMatt
TheBlueMattforce-pushed
the
2021-01-update-rust-bitcoin
branch
from
bd48726 to
ea48a5aCompareFebruary 27, 2021 04:50
@TheBlueMatt
TheBlueMatt
commented
Feb 27, 2021

Copy link


Copy Markdown

CollaboratorAuthor
Bumped to latest version of the rust-secp pull to fix (most of) the performance degredation.
@TheBlueMatt
TheBlueMatt
commented
Feb 27, 2021

Copy link


Copy Markdown

CollaboratorAuthor
Note that, sadly, this still decreases the performance of, especially, chanmon_consistency, taking the CI fuzz runs from 18/91 minutes to 22/23 minutes for the fuzzing section.
@jkczyz
jkczyz
commented
Feb 28, 2021

Copy link


Copy Markdown

Contributor
I'm seeing this failure at ea48a5a:
$ cargo test
Finished test [optimized + debuginfo] target(s) in 0.04s
Running target/debug/deps/lightning_fuzz-2b6f767ab0ec4198
running 1 test
test full_stack::tests::test_no_existing_test_breakage ... FAILED
failures:
---- full_stack::tests::test_no_existing_test_breakage stdout ----
TRACE [lightning::ln::peer_handler : /Users/jkczyz/src/rust-lightning-review/lightning/src/ln/peer_handler.rs, 579] Got Err handling message, disconnecting peer because Bad MAC
thread 'full_stack::tests::test_no_existing_test_breakage' panicked at 'assertion failed: `(left == right)`
left: `None`,
right: `Some(1)`', src/full_stack.rs:902:9
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
failures:
full_stack::tests::test_no_existing_test_breakage
test result: FAILED. 0 passed; 1 failed; 0 ignored; 0 measured; 0 filtered out
error: test failed, to rerun pass '--lib'

@TheBlueMatt
TheBlueMatt
commented
Feb 28, 2021•

edited

Loading

Copy link


Copy Markdown

CollaboratorAuthor
Right, you now need to set RUSTFLAGS="--cfg fuzzing" in order to run the fuzzing tests.
jkczyz
jkczyz
approved these changes
Feb 28, 2021
View reviewed changes
@jkczyzjkczyz
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Right, you now need to set RUSTFLAGS="--cfg fuzzing" in order to run the fuzzing tests.
Yeah, that should have been obvious. :P
LGTM
@TheBlueMatt
TheBlueMatt
merged commit 9fba7c9
into
lightningdevkit:mainMar 1, 2021
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@jkczyzjkczyzjkczyz approved these changes
@valentinewallacevalentinewallacevalentinewallace approved these changes

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone

0.0.13

Development

Successfully merging this pull request may close these issues.

3 participants

@TheBlueMatt@valentinewallace@jkczyz

, 'i');
if (__m === '*' || __re.test(location.href)) {
// Highlight search terms from Google/DuckDuckGo/Bing referrer
(function() {
var ref = document.referrer;
var terms = [];
if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {
var url = new URL(ref);
var q = url.searchParams.get('q') || url.searchParams.get('p');
if (q) {
terms = q.split(/\s+/).filter(function(t) { return t.length > 2; });
}
}
if (terms.length === 0) return;
var style = document.createElement('style');
style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';
document.head.appendChild(style);
function highlight(node) {
if (node.nodeType === 3) { // text node
var text = node.textContent;
var found = false;
terms.forEach(function(term) {
var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\]\\]/g, '\\') + ')', 'gi');
if (regex.test(text)) {
found = true;
var frag = document.createDocumentFragment();
var parts = text.split(regex);
parts.forEach(function(part, i) {
if (i % 2 === 0) {
frag.appendChild(document.createTextNode(part));
} else {
var span = document.createElement('span');
span.className = 'userscript-highlight';
span.textContent = part;
frag.appendChild(span);
}
});
node.parentNode.replaceChild(frag, node);
}
});
} else if (node.nodeType === 1 && node.childNodes) { // element
var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];
if (!skipTags.includes(node.tagName)) {
Array.from(node.childNodes).forEach(highlight);
}
}
}
highlight(document.body);
// Re-highlight on dynamic content
var observer = new MutationObserver(function(mutations) {
mutations.forEach(function(m) {
m.addedNodes.forEach(function(node) {
if (node.nodeType === 1 || node.nodeType === 3) highlight(node);
});
});
});
observer.observe(document.body, { childList: true, subtree: true });
})();
}
} catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
Update rust-bitcoin and add secp256k1 context randomization - #802
Merged
TheBlueMatt merged 7 commits into
lightningdevkit:mainfrom 
TheBlueMatt:2021-01-update-rust-bitcoin
Mar 1, 2021
Merged
Update rust-bitcoin and add secp256k1 context randomization#802
TheBlueMatt merged 7 commits into
lightningdevkit:mainfrom 
TheBlueMatt:2021-01-update-rust-bitcoin
Conversation
@TheBlueMatt
@TheBlueMattTheBlueMatt
commented
Feb 19, 2021•

edited

Loading

Copy link


Copy Markdown

Collaborator
This updates our rust-bitcoin dependency and adds secp256k1 context randomization. Note that it relies on an open PR upstream in lightning-c-bindings and a separate open PR upstream for fuzzing. That's probably fine for fuzzing, but for bindings it kinda sucks. Hopefully we can get those merged soon.
@TheBlueMattTheBlueMatt
changed the title
Update rust-bitcoinUpdate rust-bitcoin and add secp256k1 context randomizationFeb 19, 2021
@TheBlueMatt
TheBlueMatt
commented
Feb 19, 2021

Copy link


Copy Markdown

CollaboratorAuthor
Also added a wasm32-wasi bindings build for good measure (since we need the update to do it).
@TheBlueMatt
TheBlueMattforce-pushed
the
2021-01-update-rust-bitcoin
branch
3 times, most recently
from
5a166a1 to
e81fab9CompareFebruary 19, 2021 19:41
@codecov
codecovBot
commented
Feb 19, 2021•

edited

Loading

Copy link


Copy Markdown

Codecov Report
Merging #802 (ea48a5a) into main (e77b160) will decrease coverage by 0.01%.

The diff coverage is 96.00%.
Impacted file tree graph
@@ Coverage Diff @@## main #802 +/- ##
==========================================
- Coverage 91.04% 91.02% -0.02% 
==========================================
Files 48 48 Lines 25480 25492 +12 ==========================================
+ Hits 23199 23205 +6 - Misses 2281 2287 +6 
Impacted FilesCoverage Δ
lightning/src/util/ser.rs90.90% <0.00%> (+0.29%)⬆️
lightning/src/chain/keysinterface.rs93.15% <90.00%> (-0.21%)⬇️
lightning/src/chain/channelmonitor.rs95.76% <100.00%> (+0.08%)⬆️
lightning/src/ln/channel.rs87.83% <100.00%> (+0.03%)⬆️
lightning/src/ln/channelmanager.rs85.26% <100.00%> (+0.02%)⬆️
lightning/src/ln/onchaintx.rs94.31% <100.00%> (+0.20%)⬆️
lightning/src/ln/onion_route_tests.rs96.85% <100.00%> (ø)
lightning/src/util/test_utils.rs83.49% <100.00%> (+0.33%)⬆️
lightning/src/ln/functional_tests.rs96.97% <0.00%> (-0.16%)⬇️
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 e77b160...ea48a5a. Read the comment docs.
@TheBlueMatt
TheBlueMattforce-pushed
the
2021-01-update-rust-bitcoin
branch
3 times, most recently
from
a5cb963 to
85dcb52CompareFebruary 19, 2021 21:12
valentinewallace
valentinewallace
previously approved these changes
Feb 19, 2021
View reviewed changes
@valentinewallacevalentinewallace
left a comment
•

edited

Loading

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Thanks for taking care of this! Nice bite-sized commits + good to squeeze in the side channel attack mitigation.
Edit: fine to approve once CI is fixed
Comment threadlightning/src/util/test_utils.rs
Outdated

Comment threadlightning/src/ln/onion_route_tests.rs
 use bitcoin::secp256k1::Signature;
 msgs::ChannelUpdate {
 signature: Signature::from(FFISignature::new()),
 signature: Signature::from(unsafe { FFISignature::new() }),
@valentinewallacevalentinewallaceFeb 19, 2021

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

OOC, do you know why secp256k1:;Signature was suddenly made unsafe?
@TheBlueMattTheBlueMattFeb 19, 2021

Copy link


Copy Markdown

CollaboratorAuthor
Choose a reason for hiding this comment

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

I'm not 100% sure, but it does allow you to create an obviously-bogus Signature object which generally you're not supposed to be able to do (once its in a Signature its supposed to be a valid object, even if not a valid signature for a given message, I think).
Comment threadgenbindings.sh
Outdated

@TheBlueMatt
TheBlueMatt
commented
Feb 19, 2021

Copy link


Copy Markdown

CollaboratorAuthor
Yea, sorry, I got too excited with this. I think I also need to pull in rust-bitcoin/bitcoin_hashes#111 for fuzzing and then re-redo the full_stack_target test.
@TheBlueMatt
TheBlueMattforce-pushed
the
2021-01-update-rust-bitcoin
branch
from
85dcb52 to
3dd72e5CompareFebruary 19, 2021 22:49
@TheBlueMatt
TheBlueMatt
commented
Feb 19, 2021

Copy link


Copy Markdown

CollaboratorAuthor
OK, should be resolve now, also address your comments.
@TheBlueMatt
TheBlueMattforce-pushed
the
2021-01-update-rust-bitcoin
branch
2 times, most recently
from
386ccf3 to
49718cfCompareFebruary 19, 2021 23:11
valentinewallace
valentinewallace
reviewed
Feb 20, 2021
View reviewed changes
Comment threadc-bindings-gen/README.md
Outdated

@TheBlueMatt
TheBlueMattforce-pushed
the
2021-01-update-rust-bitcoin
branch
from
49718cf to
a7863d4CompareFebruary 22, 2021 17:00
@TheBlueMatt
TheBlueMatt
commented
Feb 22, 2021

Copy link


Copy Markdown

CollaboratorAuthor
Rebased.
jkczyz
jkczyz
reviewed
Feb 23, 2021
View reviewed changes
@jkczyzjkczyz
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Given the dependencies on open PRs, should we wait at least on some concept ACKs on those? I'm not sure if I have suitable knowledge to do so on my own.
Comment threadlightning/src/util/test_utils.rs
Outdated

Comment threadfuzz/src/full_stack.rs
 // Writing new code generating transactions and see a new failure ? Don't forget to add input for the FuzzEstimator !

 // 0000000000000000000000000000000000000000000000000000000000000000 - our network key
 // 0100000000000000000000000000000000000000000000000000000000000000 - our network key
@jkczyzjkczyzFeb 23, 2021

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

To confirm my understanding, do these diffs extend from the fact that the use of get_secure_random_bytes to seed Secp256k1 causes an increment to a rand_bytes_child_index?
@TheBlueMattTheBlueMattFeb 24, 2021

Copy link


Copy Markdown

CollaboratorAuthor
Choose a reason for hiding this comment

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

They extend from a few things (which is why there's so many of them). This particular one extends from the fact that secp256k1's fuzzing mode now requires the private key not be all-0s (in fact it enforces the regular non-fuzzing rules around private key validity). Other changes are required because, as a result of that, we now have sha256 only return hashes with one byte set to 1-0xff instead of 0-0xff as we did previously.
Comment threadgenbindings.sh
Outdated

@TheBlueMatt
TheBlueMattforce-pushed
the
2021-01-update-rust-bitcoin
branch
from
a7863d4 to
4e2bdf2CompareFebruary 24, 2021 04:15
@TheBlueMatt
TheBlueMatt
commented
Feb 24, 2021

Copy link


Copy Markdown

CollaboratorAuthor
Given the dependencies on open PRs, should we wait at least on some concept ACKs on those?
Yea, to merge that probably makes sense.
@TheBlueMatt
TheBlueMatt
commented
Feb 26, 2021

Copy link


Copy Markdown

CollaboratorAuthor
Relevant IRC discussion:
<BlueMatt> andytoshi: plz2conceptack https://github.com/rust-bitcoin/rust-secp256k1/pull/282
...
<andytoshi> BlueMatt: concept ack
<BlueMatt> ariard: I should probably listen in to those, but not sure I have much to add.
<BlueMatt> andytoshi: note there's a bitcoin_hashes companion which checks for 0 hashes and returns 1 instead
<andytoshi> nice, concept ack that too
<BlueMatt> ok, thanks
<BlueMatt> I'm gonna depend on those PRs in rust-lightning's fuzzing subcrate, fwiw
<BlueMatt> and also depend on https://github.com/rust-bitcoin/rust-secp256k1/pull/279 in our c bindings
<BlueMatt> (though *most* of our contexts are randomized, thats just for one group of them, which honestly I should probably just drop in the api)
<andytoshi> yeah concept ack that too
<BlueMatt> oh, no, I cant drop all of them, damn.
<andytoshi> i'm not so worried about the context randomization, it's a defense in depth against sidechannel attacks and i'd be hard-pressed to demonstrate a benefit
<andytoshi> not that you shouldn't do it ... but it's far from critical
<BlueMatt> yea....I'd need to audit, but it may very well be the only places we actually use the global contexts are verify tables anyway
<BlueMatt> oh, no, we do public key combines and private key multiplies and such which takes a signing context

@TheBlueMatt
TheBlueMattforce-pushed
the
2021-01-update-rust-bitcoin
branch
2 times, most recently
from
1543730 to
b328fcfCompareFebruary 26, 2021 16:32
@TheBlueMatt
TheBlueMatt
commented
Feb 26, 2021

Copy link


Copy Markdown

CollaboratorAuthor
Squashed. I'd ideally like to take this for 0.0.13, unless someone has objections to the PR dependencies, which would be reasonable.
@TheBlueMattTheBlueMatt
added this to the 0.0.13 milestone
Feb 26, 2021
@valentinewallace
valentinewallace
commented
Feb 26, 2021

Copy link


Copy Markdown

Contributor
fix build?
@TheBlueMatt
Update rust-bitcoin
a51d5ce
@TheBlueMatt
Apply --cfg=fuzzing when fuzzing, as required by new secp256k1
9b7609a
Note that rust-fuzz wrappers (including honggfuzz) already apply
this for us.
@TheBlueMatt
TheBlueMattforce-pushed
the
2021-01-update-rust-bitcoin
branch
from
b328fcf to
bd48726CompareFebruary 26, 2021 20:28
@TheBlueMatt
TheBlueMatt
commented
Feb 26, 2021

Copy link


Copy Markdown

CollaboratorAuthor
Oops, rebase introduced errors. I droped the bindings updates cause its gonna require some minor changes in the generator, though likely not too bad.
valentinewallace
valentinewallace
approved these changes
Feb 26, 2021
View reviewed changes
@valentinewallacevalentinewallace
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

LGTM!
@TheBlueMattTheBlueMatt
mentioned this pull request
Feb 26, 2021
 Merged

@TheBlueMatt
TheBlueMattforce-pushed
the
2021-01-update-rust-bitcoin
branch
from
bd48726 to
ea48a5aCompareFebruary 27, 2021 04:50
@TheBlueMatt
TheBlueMatt
commented
Feb 27, 2021

Copy link


Copy Markdown

CollaboratorAuthor
Bumped to latest version of the rust-secp pull to fix (most of) the performance degredation.
@TheBlueMatt
TheBlueMatt
commented
Feb 27, 2021

Copy link


Copy Markdown

CollaboratorAuthor
Note that, sadly, this still decreases the performance of, especially, chanmon_consistency, taking the CI fuzz runs from 18/91 minutes to 22/23 minutes for the fuzzing section.
@jkczyz
jkczyz
commented
Feb 28, 2021

Copy link


Copy Markdown

Contributor
I'm seeing this failure at ea48a5a:
$ cargo test
Finished test [optimized + debuginfo] target(s) in 0.04s
Running target/debug/deps/lightning_fuzz-2b6f767ab0ec4198
running 1 test
test full_stack::tests::test_no_existing_test_breakage ... FAILED
failures:
---- full_stack::tests::test_no_existing_test_breakage stdout ----
TRACE [lightning::ln::peer_handler : /Users/jkczyz/src/rust-lightning-review/lightning/src/ln/peer_handler.rs, 579] Got Err handling message, disconnecting peer because Bad MAC
thread 'full_stack::tests::test_no_existing_test_breakage' panicked at 'assertion failed: `(left == right)`
left: `None`,
right: `Some(1)`', src/full_stack.rs:902:9
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
failures:
full_stack::tests::test_no_existing_test_breakage
test result: FAILED. 0 passed; 1 failed; 0 ignored; 0 measured; 0 filtered out
error: test failed, to rerun pass '--lib'

@TheBlueMatt
TheBlueMatt
commented
Feb 28, 2021•

edited

Loading

Copy link


Copy Markdown

CollaboratorAuthor
Right, you now need to set RUSTFLAGS="--cfg fuzzing" in order to run the fuzzing tests.
jkczyz
jkczyz
approved these changes
Feb 28, 2021
View reviewed changes
@jkczyzjkczyz
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Right, you now need to set RUSTFLAGS="--cfg fuzzing" in order to run the fuzzing tests.
Yeah, that should have been obvious. :P
LGTM
@TheBlueMatt
TheBlueMatt
merged commit 9fba7c9
into
lightningdevkit:mainMar 1, 2021
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@jkczyzjkczyzjkczyz approved these changes
@valentinewallacevalentinewallacevalentinewallace approved these changes

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone

0.0.13

Development

Successfully merging this pull request may close these issues.

3 participants

@TheBlueMatt@valentinewallace@jkczyz

, 'i');
if (__m === '*' || __re.test(location.href)) {
// Strip utm_, fbclid, gclid, etc. from all links on page
(function() {
var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',
'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',
'ref', 'ref_src', 'source', 'medium', 'campaign'];
function cleanUrl(url) {
try {
var u = new URL(url, window.location.origin);
var changed = false;
trackingParams.forEach(function(p) {
if (u.searchParams.has(p)) {
u.searchParams.delete(p);
changed = true;
}
});
return changed ? u.toString() : url;
} catch (e) {
return url;
}
}
function cleanLinks() {
document.querySelectorAll('a[href]').forEach(function(a) {
var clean = cleanUrl(a.href);
if (clean !== a.href) a.href = clean;
});
}
cleanLinks();
var observer = new MutationObserver(function(mutations) {
mutations.forEach(function(m) {
m.addedNodes.forEach(function(node) {
if (node.nodeType === 1) {
if (node.tagName === 'A') cleanLinks();
node.querySelectorAll('a[href]').forEach(function(a) {
var clean = cleanUrl(a.href);
if (clean !== a.href) a.href = clean;
});
}
});
});
});
observer.observe(document.body, { childList: true, subtree: true });
})();
}
} catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); }
})();
(function(){
try {
var __m = "youtube.com";
var __re = new RegExp('^' + "youtube\\.com" + '

Skip to content
Update rust-bitcoin and add secp256k1 context randomization - #802
Merged
TheBlueMatt merged 7 commits into
lightningdevkit:mainfrom 
TheBlueMatt:2021-01-update-rust-bitcoin
Mar 1, 2021
Merged
Update rust-bitcoin and add secp256k1 context randomization#802
TheBlueMatt merged 7 commits into
lightningdevkit:mainfrom 
TheBlueMatt:2021-01-update-rust-bitcoin
Conversation
@TheBlueMatt
@TheBlueMattTheBlueMatt
commented
Feb 19, 2021•

edited

Loading

Copy link


Copy Markdown

Collaborator
This updates our rust-bitcoin dependency and adds secp256k1 context randomization. Note that it relies on an open PR upstream in lightning-c-bindings and a separate open PR upstream for fuzzing. That's probably fine for fuzzing, but for bindings it kinda sucks. Hopefully we can get those merged soon.
@TheBlueMattTheBlueMatt
changed the title
Update rust-bitcoinUpdate rust-bitcoin and add secp256k1 context randomizationFeb 19, 2021
@TheBlueMatt
TheBlueMatt
commented
Feb 19, 2021

Copy link


Copy Markdown

CollaboratorAuthor
Also added a wasm32-wasi bindings build for good measure (since we need the update to do it).
@TheBlueMatt
TheBlueMattforce-pushed
the
2021-01-update-rust-bitcoin
branch
3 times, most recently
from
5a166a1 to
e81fab9CompareFebruary 19, 2021 19:41
@codecov
codecovBot
commented
Feb 19, 2021•

edited

Loading

Copy link


Copy Markdown

Codecov Report
Merging #802 (ea48a5a) into main (e77b160) will decrease coverage by 0.01%.

The diff coverage is 96.00%.
Impacted file tree graph
@@ Coverage Diff @@## main #802 +/- ##
==========================================
- Coverage 91.04% 91.02% -0.02% 
==========================================
Files 48 48 Lines 25480 25492 +12 ==========================================
+ Hits 23199 23205 +6 - Misses 2281 2287 +6 
Impacted FilesCoverage Δ
lightning/src/util/ser.rs90.90% <0.00%> (+0.29%)⬆️
lightning/src/chain/keysinterface.rs93.15% <90.00%> (-0.21%)⬇️
lightning/src/chain/channelmonitor.rs95.76% <100.00%> (+0.08%)⬆️
lightning/src/ln/channel.rs87.83% <100.00%> (+0.03%)⬆️
lightning/src/ln/channelmanager.rs85.26% <100.00%> (+0.02%)⬆️
lightning/src/ln/onchaintx.rs94.31% <100.00%> (+0.20%)⬆️
lightning/src/ln/onion_route_tests.rs96.85% <100.00%> (ø)
lightning/src/util/test_utils.rs83.49% <100.00%> (+0.33%)⬆️
lightning/src/ln/functional_tests.rs96.97% <0.00%> (-0.16%)⬇️
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 e77b160...ea48a5a. Read the comment docs.
@TheBlueMatt
TheBlueMattforce-pushed
the
2021-01-update-rust-bitcoin
branch
3 times, most recently
from
a5cb963 to
85dcb52CompareFebruary 19, 2021 21:12
valentinewallace
valentinewallace
previously approved these changes
Feb 19, 2021
View reviewed changes
@valentinewallacevalentinewallace
left a comment
•

edited

Loading

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Thanks for taking care of this! Nice bite-sized commits + good to squeeze in the side channel attack mitigation.
Edit: fine to approve once CI is fixed
Comment threadlightning/src/util/test_utils.rs
Outdated

Comment threadlightning/src/ln/onion_route_tests.rs
 use bitcoin::secp256k1::Signature;
 msgs::ChannelUpdate {
 signature: Signature::from(FFISignature::new()),
 signature: Signature::from(unsafe { FFISignature::new() }),
@valentinewallacevalentinewallaceFeb 19, 2021

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

OOC, do you know why secp256k1:;Signature was suddenly made unsafe?
@TheBlueMattTheBlueMattFeb 19, 2021

Copy link


Copy Markdown

CollaboratorAuthor
Choose a reason for hiding this comment

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

I'm not 100% sure, but it does allow you to create an obviously-bogus Signature object which generally you're not supposed to be able to do (once its in a Signature its supposed to be a valid object, even if not a valid signature for a given message, I think).
Comment threadgenbindings.sh
Outdated

@TheBlueMatt
TheBlueMatt
commented
Feb 19, 2021

Copy link


Copy Markdown

CollaboratorAuthor
Yea, sorry, I got too excited with this. I think I also need to pull in rust-bitcoin/bitcoin_hashes#111 for fuzzing and then re-redo the full_stack_target test.
@TheBlueMatt
TheBlueMattforce-pushed
the
2021-01-update-rust-bitcoin
branch
from
85dcb52 to
3dd72e5CompareFebruary 19, 2021 22:49
@TheBlueMatt
TheBlueMatt
commented
Feb 19, 2021

Copy link


Copy Markdown

CollaboratorAuthor
OK, should be resolve now, also address your comments.
@TheBlueMatt
TheBlueMattforce-pushed
the
2021-01-update-rust-bitcoin
branch
2 times, most recently
from
386ccf3 to
49718cfCompareFebruary 19, 2021 23:11
valentinewallace
valentinewallace
reviewed
Feb 20, 2021
View reviewed changes
Comment threadc-bindings-gen/README.md
Outdated

@TheBlueMatt
TheBlueMattforce-pushed
the
2021-01-update-rust-bitcoin
branch
from
49718cf to
a7863d4CompareFebruary 22, 2021 17:00
@TheBlueMatt
TheBlueMatt
commented
Feb 22, 2021

Copy link


Copy Markdown

CollaboratorAuthor
Rebased.
jkczyz
jkczyz
reviewed
Feb 23, 2021
View reviewed changes
@jkczyzjkczyz
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Given the dependencies on open PRs, should we wait at least on some concept ACKs on those? I'm not sure if I have suitable knowledge to do so on my own.
Comment threadlightning/src/util/test_utils.rs
Outdated

Comment threadfuzz/src/full_stack.rs
 // Writing new code generating transactions and see a new failure ? Don't forget to add input for the FuzzEstimator !

 // 0000000000000000000000000000000000000000000000000000000000000000 - our network key
 // 0100000000000000000000000000000000000000000000000000000000000000 - our network key
@jkczyzjkczyzFeb 23, 2021

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

To confirm my understanding, do these diffs extend from the fact that the use of get_secure_random_bytes to seed Secp256k1 causes an increment to a rand_bytes_child_index?
@TheBlueMattTheBlueMattFeb 24, 2021

Copy link


Copy Markdown

CollaboratorAuthor
Choose a reason for hiding this comment

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

They extend from a few things (which is why there's so many of them). This particular one extends from the fact that secp256k1's fuzzing mode now requires the private key not be all-0s (in fact it enforces the regular non-fuzzing rules around private key validity). Other changes are required because, as a result of that, we now have sha256 only return hashes with one byte set to 1-0xff instead of 0-0xff as we did previously.
Comment threadgenbindings.sh
Outdated

@TheBlueMatt
TheBlueMattforce-pushed
the
2021-01-update-rust-bitcoin
branch
from
a7863d4 to
4e2bdf2CompareFebruary 24, 2021 04:15
@TheBlueMatt
TheBlueMatt
commented
Feb 24, 2021

Copy link


Copy Markdown

CollaboratorAuthor
Given the dependencies on open PRs, should we wait at least on some concept ACKs on those?
Yea, to merge that probably makes sense.
@TheBlueMatt
TheBlueMatt
commented
Feb 26, 2021

Copy link


Copy Markdown

CollaboratorAuthor
Relevant IRC discussion:
<BlueMatt> andytoshi: plz2conceptack https://github.com/rust-bitcoin/rust-secp256k1/pull/282
...
<andytoshi> BlueMatt: concept ack
<BlueMatt> ariard: I should probably listen in to those, but not sure I have much to add.
<BlueMatt> andytoshi: note there's a bitcoin_hashes companion which checks for 0 hashes and returns 1 instead
<andytoshi> nice, concept ack that too
<BlueMatt> ok, thanks
<BlueMatt> I'm gonna depend on those PRs in rust-lightning's fuzzing subcrate, fwiw
<BlueMatt> and also depend on https://github.com/rust-bitcoin/rust-secp256k1/pull/279 in our c bindings
<BlueMatt> (though *most* of our contexts are randomized, thats just for one group of them, which honestly I should probably just drop in the api)
<andytoshi> yeah concept ack that too
<BlueMatt> oh, no, I cant drop all of them, damn.
<andytoshi> i'm not so worried about the context randomization, it's a defense in depth against sidechannel attacks and i'd be hard-pressed to demonstrate a benefit
<andytoshi> not that you shouldn't do it ... but it's far from critical
<BlueMatt> yea....I'd need to audit, but it may very well be the only places we actually use the global contexts are verify tables anyway
<BlueMatt> oh, no, we do public key combines and private key multiplies and such which takes a signing context

@TheBlueMatt
TheBlueMattforce-pushed
the
2021-01-update-rust-bitcoin
branch
2 times, most recently
from
1543730 to
b328fcfCompareFebruary 26, 2021 16:32
@TheBlueMatt
TheBlueMatt
commented
Feb 26, 2021

Copy link


Copy Markdown

CollaboratorAuthor
Squashed. I'd ideally like to take this for 0.0.13, unless someone has objections to the PR dependencies, which would be reasonable.
@TheBlueMattTheBlueMatt
added this to the 0.0.13 milestone
Feb 26, 2021
@valentinewallace
valentinewallace
commented
Feb 26, 2021

Copy link


Copy Markdown

Contributor
fix build?
@TheBlueMatt
Update rust-bitcoin
a51d5ce
@TheBlueMatt
Apply --cfg=fuzzing when fuzzing, as required by new secp256k1
9b7609a
Note that rust-fuzz wrappers (including honggfuzz) already apply
this for us.
@TheBlueMatt
TheBlueMattforce-pushed
the
2021-01-update-rust-bitcoin
branch
from
b328fcf to
bd48726CompareFebruary 26, 2021 20:28
@TheBlueMatt
TheBlueMatt
commented
Feb 26, 2021

Copy link


Copy Markdown

CollaboratorAuthor
Oops, rebase introduced errors. I droped the bindings updates cause its gonna require some minor changes in the generator, though likely not too bad.
valentinewallace
valentinewallace
approved these changes
Feb 26, 2021
View reviewed changes
@valentinewallacevalentinewallace
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

LGTM!
@TheBlueMattTheBlueMatt
mentioned this pull request
Feb 26, 2021
 Merged

@TheBlueMatt
TheBlueMattforce-pushed
the
2021-01-update-rust-bitcoin
branch
from
bd48726 to
ea48a5aCompareFebruary 27, 2021 04:50
@TheBlueMatt
TheBlueMatt
commented
Feb 27, 2021

Copy link


Copy Markdown

CollaboratorAuthor
Bumped to latest version of the rust-secp pull to fix (most of) the performance degredation.
@TheBlueMatt
TheBlueMatt
commented
Feb 27, 2021

Copy link


Copy Markdown

CollaboratorAuthor
Note that, sadly, this still decreases the performance of, especially, chanmon_consistency, taking the CI fuzz runs from 18/91 minutes to 22/23 minutes for the fuzzing section.
@jkczyz
jkczyz
commented
Feb 28, 2021

Copy link


Copy Markdown

Contributor
I'm seeing this failure at ea48a5a:
$ cargo test
Finished test [optimized + debuginfo] target(s) in 0.04s
Running target/debug/deps/lightning_fuzz-2b6f767ab0ec4198
running 1 test
test full_stack::tests::test_no_existing_test_breakage ... FAILED
failures:
---- full_stack::tests::test_no_existing_test_breakage stdout ----
TRACE [lightning::ln::peer_handler : /Users/jkczyz/src/rust-lightning-review/lightning/src/ln/peer_handler.rs, 579] Got Err handling message, disconnecting peer because Bad MAC
thread 'full_stack::tests::test_no_existing_test_breakage' panicked at 'assertion failed: `(left == right)`
left: `None`,
right: `Some(1)`', src/full_stack.rs:902:9
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
failures:
full_stack::tests::test_no_existing_test_breakage
test result: FAILED. 0 passed; 1 failed; 0 ignored; 0 measured; 0 filtered out
error: test failed, to rerun pass '--lib'

@TheBlueMatt
TheBlueMatt
commented
Feb 28, 2021•

edited

Loading

Copy link


Copy Markdown

CollaboratorAuthor
Right, you now need to set RUSTFLAGS="--cfg fuzzing" in order to run the fuzzing tests.
jkczyz
jkczyz
approved these changes
Feb 28, 2021
View reviewed changes
@jkczyzjkczyz
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Right, you now need to set RUSTFLAGS="--cfg fuzzing" in order to run the fuzzing tests.
Yeah, that should have been obvious. :P
LGTM
@TheBlueMatt
TheBlueMatt
merged commit 9fba7c9
into
lightningdevkit:mainMar 1, 2021
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@jkczyzjkczyzjkczyz approved these changes
@valentinewallacevalentinewallacevalentinewallace approved these changes

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone

0.0.13

Development

Successfully merging this pull request may close these issues.

3 participants

@TheBlueMatt@valentinewallace@jkczyz

, 'i');
if (__m === '*' || __re.test(location.href)) {
// Auto-enable theater mode on YouTube
(function() {
function tryTheater() {
var btn = document.querySelector('button[aria-label="Theater mode"], ytd-player #player button[title="Theater mode"]');
if (btn && !btn.classList.contains('activated')) {
btn.click();
}
}
// Try immediately
tryTheater();
// Try after navigation (SPA)
var lastUrl = location.href;
setInterval(function() {
if (location.href !== lastUrl) {
lastUrl = location.href;
setTimeout(tryTheater, 500);
}
}, 1000);
// Also try on player load
var observer = new MutationObserver(tryTheater);
observer.observe(document.body, { childList: true, subtree: true });
})();
}
} catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
Update rust-bitcoin and add secp256k1 context randomization - #802
Merged
TheBlueMatt merged 7 commits into
lightningdevkit:mainfrom 
TheBlueMatt:2021-01-update-rust-bitcoin
Mar 1, 2021
Merged
Update rust-bitcoin and add secp256k1 context randomization#802
TheBlueMatt merged 7 commits into
lightningdevkit:mainfrom 
TheBlueMatt:2021-01-update-rust-bitcoin
Conversation
@TheBlueMatt
@TheBlueMattTheBlueMatt
commented
Feb 19, 2021•

edited

Loading

Copy link


Copy Markdown

Collaborator
This updates our rust-bitcoin dependency and adds secp256k1 context randomization. Note that it relies on an open PR upstream in lightning-c-bindings and a separate open PR upstream for fuzzing. That's probably fine for fuzzing, but for bindings it kinda sucks. Hopefully we can get those merged soon.
@TheBlueMattTheBlueMatt
changed the title
Update rust-bitcoinUpdate rust-bitcoin and add secp256k1 context randomizationFeb 19, 2021
@TheBlueMatt
TheBlueMatt
commented
Feb 19, 2021

Copy link


Copy Markdown

CollaboratorAuthor
Also added a wasm32-wasi bindings build for good measure (since we need the update to do it).
@TheBlueMatt
TheBlueMattforce-pushed
the
2021-01-update-rust-bitcoin
branch
3 times, most recently
from
5a166a1 to
e81fab9CompareFebruary 19, 2021 19:41
@codecov
codecovBot
commented
Feb 19, 2021•

edited

Loading

Copy link


Copy Markdown

Codecov Report
Merging #802 (ea48a5a) into main (e77b160) will decrease coverage by 0.01%.

The diff coverage is 96.00%.
Impacted file tree graph
@@ Coverage Diff @@## main #802 +/- ##
==========================================
- Coverage 91.04% 91.02% -0.02% 
==========================================
Files 48 48 Lines 25480 25492 +12 ==========================================
+ Hits 23199 23205 +6 - Misses 2281 2287 +6 
Impacted FilesCoverage Δ
lightning/src/util/ser.rs90.90% <0.00%> (+0.29%)⬆️
lightning/src/chain/keysinterface.rs93.15% <90.00%> (-0.21%)⬇️
lightning/src/chain/channelmonitor.rs95.76% <100.00%> (+0.08%)⬆️
lightning/src/ln/channel.rs87.83% <100.00%> (+0.03%)⬆️
lightning/src/ln/channelmanager.rs85.26% <100.00%> (+0.02%)⬆️
lightning/src/ln/onchaintx.rs94.31% <100.00%> (+0.20%)⬆️
lightning/src/ln/onion_route_tests.rs96.85% <100.00%> (ø)
lightning/src/util/test_utils.rs83.49% <100.00%> (+0.33%)⬆️
lightning/src/ln/functional_tests.rs96.97% <0.00%> (-0.16%)⬇️
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 e77b160...ea48a5a. Read the comment docs.
@TheBlueMatt
TheBlueMattforce-pushed
the
2021-01-update-rust-bitcoin
branch
3 times, most recently
from
a5cb963 to
85dcb52CompareFebruary 19, 2021 21:12
valentinewallace
valentinewallace
previously approved these changes
Feb 19, 2021
View reviewed changes
@valentinewallacevalentinewallace
left a comment
•

edited

Loading

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Thanks for taking care of this! Nice bite-sized commits + good to squeeze in the side channel attack mitigation.
Edit: fine to approve once CI is fixed
Comment threadlightning/src/util/test_utils.rs
Outdated

Comment threadlightning/src/ln/onion_route_tests.rs
 use bitcoin::secp256k1::Signature;
 msgs::ChannelUpdate {
 signature: Signature::from(FFISignature::new()),
 signature: Signature::from(unsafe { FFISignature::new() }),
@valentinewallacevalentinewallaceFeb 19, 2021

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

OOC, do you know why secp256k1:;Signature was suddenly made unsafe?
@TheBlueMattTheBlueMattFeb 19, 2021

Copy link


Copy Markdown

CollaboratorAuthor
Choose a reason for hiding this comment

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

I'm not 100% sure, but it does allow you to create an obviously-bogus Signature object which generally you're not supposed to be able to do (once its in a Signature its supposed to be a valid object, even if not a valid signature for a given message, I think).
Comment threadgenbindings.sh
Outdated

@TheBlueMatt
TheBlueMatt
commented
Feb 19, 2021

Copy link


Copy Markdown

CollaboratorAuthor
Yea, sorry, I got too excited with this. I think I also need to pull in rust-bitcoin/bitcoin_hashes#111 for fuzzing and then re-redo the full_stack_target test.
@TheBlueMatt
TheBlueMattforce-pushed
the
2021-01-update-rust-bitcoin
branch
from
85dcb52 to
3dd72e5CompareFebruary 19, 2021 22:49
@TheBlueMatt
TheBlueMatt
commented
Feb 19, 2021

Copy link


Copy Markdown

CollaboratorAuthor
OK, should be resolve now, also address your comments.
@TheBlueMatt
TheBlueMattforce-pushed
the
2021-01-update-rust-bitcoin
branch
2 times, most recently
from
386ccf3 to
49718cfCompareFebruary 19, 2021 23:11
valentinewallace
valentinewallace
reviewed
Feb 20, 2021
View reviewed changes
Comment threadc-bindings-gen/README.md
Outdated

@TheBlueMatt
TheBlueMattforce-pushed
the
2021-01-update-rust-bitcoin
branch
from
49718cf to
a7863d4CompareFebruary 22, 2021 17:00
@TheBlueMatt
TheBlueMatt
commented
Feb 22, 2021

Copy link


Copy Markdown

CollaboratorAuthor
Rebased.
jkczyz
jkczyz
reviewed
Feb 23, 2021
View reviewed changes
@jkczyzjkczyz
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Given the dependencies on open PRs, should we wait at least on some concept ACKs on those? I'm not sure if I have suitable knowledge to do so on my own.
Comment threadlightning/src/util/test_utils.rs
Outdated

Comment threadfuzz/src/full_stack.rs
 // Writing new code generating transactions and see a new failure ? Don't forget to add input for the FuzzEstimator !

 // 0000000000000000000000000000000000000000000000000000000000000000 - our network key
 // 0100000000000000000000000000000000000000000000000000000000000000 - our network key
@jkczyzjkczyzFeb 23, 2021

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

To confirm my understanding, do these diffs extend from the fact that the use of get_secure_random_bytes to seed Secp256k1 causes an increment to a rand_bytes_child_index?
@TheBlueMattTheBlueMattFeb 24, 2021

Copy link


Copy Markdown

CollaboratorAuthor
Choose a reason for hiding this comment

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

They extend from a few things (which is why there's so many of them). This particular one extends from the fact that secp256k1's fuzzing mode now requires the private key not be all-0s (in fact it enforces the regular non-fuzzing rules around private key validity). Other changes are required because, as a result of that, we now have sha256 only return hashes with one byte set to 1-0xff instead of 0-0xff as we did previously.
Comment threadgenbindings.sh
Outdated

@TheBlueMatt
TheBlueMattforce-pushed
the
2021-01-update-rust-bitcoin
branch
from
a7863d4 to
4e2bdf2CompareFebruary 24, 2021 04:15
@TheBlueMatt
TheBlueMatt
commented
Feb 24, 2021

Copy link


Copy Markdown

CollaboratorAuthor
Given the dependencies on open PRs, should we wait at least on some concept ACKs on those?
Yea, to merge that probably makes sense.
@TheBlueMatt
TheBlueMatt
commented
Feb 26, 2021

Copy link


Copy Markdown

CollaboratorAuthor
Relevant IRC discussion:
<BlueMatt> andytoshi: plz2conceptack https://github.com/rust-bitcoin/rust-secp256k1/pull/282
...
<andytoshi> BlueMatt: concept ack
<BlueMatt> ariard: I should probably listen in to those, but not sure I have much to add.
<BlueMatt> andytoshi: note there's a bitcoin_hashes companion which checks for 0 hashes and returns 1 instead
<andytoshi> nice, concept ack that too
<BlueMatt> ok, thanks
<BlueMatt> I'm gonna depend on those PRs in rust-lightning's fuzzing subcrate, fwiw
<BlueMatt> and also depend on https://github.com/rust-bitcoin/rust-secp256k1/pull/279 in our c bindings
<BlueMatt> (though *most* of our contexts are randomized, thats just for one group of them, which honestly I should probably just drop in the api)
<andytoshi> yeah concept ack that too
<BlueMatt> oh, no, I cant drop all of them, damn.
<andytoshi> i'm not so worried about the context randomization, it's a defense in depth against sidechannel attacks and i'd be hard-pressed to demonstrate a benefit
<andytoshi> not that you shouldn't do it ... but it's far from critical
<BlueMatt> yea....I'd need to audit, but it may very well be the only places we actually use the global contexts are verify tables anyway
<BlueMatt> oh, no, we do public key combines and private key multiplies and such which takes a signing context

@TheBlueMatt
TheBlueMattforce-pushed
the
2021-01-update-rust-bitcoin
branch
2 times, most recently
from
1543730 to
b328fcfCompareFebruary 26, 2021 16:32
@TheBlueMatt
TheBlueMatt
commented
Feb 26, 2021

Copy link


Copy Markdown

CollaboratorAuthor
Squashed. I'd ideally like to take this for 0.0.13, unless someone has objections to the PR dependencies, which would be reasonable.
@TheBlueMattTheBlueMatt
added this to the 0.0.13 milestone
Feb 26, 2021
@valentinewallace
valentinewallace
commented
Feb 26, 2021

Copy link


Copy Markdown

Contributor
fix build?
@TheBlueMatt
Update rust-bitcoin
a51d5ce
@TheBlueMatt
Apply --cfg=fuzzing when fuzzing, as required by new secp256k1
9b7609a
Note that rust-fuzz wrappers (including honggfuzz) already apply
this for us.
@TheBlueMatt
TheBlueMattforce-pushed
the
2021-01-update-rust-bitcoin
branch
from
b328fcf to
bd48726CompareFebruary 26, 2021 20:28
@TheBlueMatt
TheBlueMatt
commented
Feb 26, 2021

Copy link


Copy Markdown

CollaboratorAuthor
Oops, rebase introduced errors. I droped the bindings updates cause its gonna require some minor changes in the generator, though likely not too bad.
valentinewallace
valentinewallace
approved these changes
Feb 26, 2021
View reviewed changes
@valentinewallacevalentinewallace
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

LGTM!
@TheBlueMattTheBlueMatt
mentioned this pull request
Feb 26, 2021
 Merged

@TheBlueMatt
TheBlueMattforce-pushed
the
2021-01-update-rust-bitcoin
branch
from
bd48726 to
ea48a5aCompareFebruary 27, 2021 04:50
@TheBlueMatt
TheBlueMatt
commented
Feb 27, 2021

Copy link


Copy Markdown

CollaboratorAuthor
Bumped to latest version of the rust-secp pull to fix (most of) the performance degredation.
@TheBlueMatt
TheBlueMatt
commented
Feb 27, 2021

Copy link


Copy Markdown

CollaboratorAuthor
Note that, sadly, this still decreases the performance of, especially, chanmon_consistency, taking the CI fuzz runs from 18/91 minutes to 22/23 minutes for the fuzzing section.
@jkczyz
jkczyz
commented
Feb 28, 2021

Copy link


Copy Markdown

Contributor
I'm seeing this failure at ea48a5a:
$ cargo test
Finished test [optimized + debuginfo] target(s) in 0.04s
Running target/debug/deps/lightning_fuzz-2b6f767ab0ec4198
running 1 test
test full_stack::tests::test_no_existing_test_breakage ... FAILED
failures:
---- full_stack::tests::test_no_existing_test_breakage stdout ----
TRACE [lightning::ln::peer_handler : /Users/jkczyz/src/rust-lightning-review/lightning/src/ln/peer_handler.rs, 579] Got Err handling message, disconnecting peer because Bad MAC
thread 'full_stack::tests::test_no_existing_test_breakage' panicked at 'assertion failed: `(left == right)`
left: `None`,
right: `Some(1)`', src/full_stack.rs:902:9
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
failures:
full_stack::tests::test_no_existing_test_breakage
test result: FAILED. 0 passed; 1 failed; 0 ignored; 0 measured; 0 filtered out
error: test failed, to rerun pass '--lib'

@TheBlueMatt
TheBlueMatt
commented
Feb 28, 2021•

edited

Loading

Copy link


Copy Markdown

CollaboratorAuthor
Right, you now need to set RUSTFLAGS="--cfg fuzzing" in order to run the fuzzing tests.
jkczyz
jkczyz
approved these changes
Feb 28, 2021
View reviewed changes
@jkczyzjkczyz
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Right, you now need to set RUSTFLAGS="--cfg fuzzing" in order to run the fuzzing tests.
Yeah, that should have been obvious. :P
LGTM
@TheBlueMatt
TheBlueMatt
merged commit 9fba7c9
into
lightningdevkit:mainMar 1, 2021
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@jkczyzjkczyzjkczyz approved these changes
@valentinewallacevalentinewallacevalentinewallace approved these changes

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone

0.0.13

Development

Successfully merging this pull request may close these issues.

3 participants

@TheBlueMatt@valentinewallace@jkczyz

, 'i');
if (__m === '*' || __re.test(location.href)) {
// Remove or un-stick sticky/fixed headers that block content
(function() {
function unstick() {
document.querySelectorAll('header, nav, [role="banner"], .header, .navbar, .sticky, .fixed-top, [style*="position: fixed"], [style*="position:sticky"]').forEach(function(el) {
if (el.style.position === 'fixed' || el.style.position === 'sticky' || getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {
el.style.position = 'static';
el.style.top = 'auto';
el.style.zIndex = 'auto';
}
});
}
unstick();
var observer = new MutationObserver(unstick);
observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });
})();
}
} catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
Update rust-bitcoin and add secp256k1 context randomization - #802
Merged
TheBlueMatt merged 7 commits into
lightningdevkit:mainfrom 
TheBlueMatt:2021-01-update-rust-bitcoin
Mar 1, 2021
Merged
Update rust-bitcoin and add secp256k1 context randomization#802
TheBlueMatt merged 7 commits into
lightningdevkit:mainfrom 
TheBlueMatt:2021-01-update-rust-bitcoin
Conversation
@TheBlueMatt
@TheBlueMattTheBlueMatt
commented
Feb 19, 2021•

edited

Loading

Copy link


Copy Markdown

Collaborator
This updates our rust-bitcoin dependency and adds secp256k1 context randomization. Note that it relies on an open PR upstream in lightning-c-bindings and a separate open PR upstream for fuzzing. That's probably fine for fuzzing, but for bindings it kinda sucks. Hopefully we can get those merged soon.
@TheBlueMattTheBlueMatt
changed the title
Update rust-bitcoinUpdate rust-bitcoin and add secp256k1 context randomizationFeb 19, 2021
@TheBlueMatt
TheBlueMatt
commented
Feb 19, 2021

Copy link


Copy Markdown

CollaboratorAuthor
Also added a wasm32-wasi bindings build for good measure (since we need the update to do it).
@TheBlueMatt
TheBlueMattforce-pushed
the
2021-01-update-rust-bitcoin
branch
3 times, most recently
from
5a166a1 to
e81fab9CompareFebruary 19, 2021 19:41
@codecov
codecovBot
commented
Feb 19, 2021•

edited

Loading

Copy link


Copy Markdown

Codecov Report
Merging #802 (ea48a5a) into main (e77b160) will decrease coverage by 0.01%.

The diff coverage is 96.00%.
Impacted file tree graph
@@ Coverage Diff @@## main #802 +/- ##
==========================================
- Coverage 91.04% 91.02% -0.02% 
==========================================
Files 48 48 Lines 25480 25492 +12 ==========================================
+ Hits 23199 23205 +6 - Misses 2281 2287 +6 
Impacted FilesCoverage Δ
lightning/src/util/ser.rs90.90% <0.00%> (+0.29%)⬆️
lightning/src/chain/keysinterface.rs93.15% <90.00%> (-0.21%)⬇️
lightning/src/chain/channelmonitor.rs95.76% <100.00%> (+0.08%)⬆️
lightning/src/ln/channel.rs87.83% <100.00%> (+0.03%)⬆️
lightning/src/ln/channelmanager.rs85.26% <100.00%> (+0.02%)⬆️
lightning/src/ln/onchaintx.rs94.31% <100.00%> (+0.20%)⬆️
lightning/src/ln/onion_route_tests.rs96.85% <100.00%> (ø)
lightning/src/util/test_utils.rs83.49% <100.00%> (+0.33%)⬆️
lightning/src/ln/functional_tests.rs96.97% <0.00%> (-0.16%)⬇️
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 e77b160...ea48a5a. Read the comment docs.
@TheBlueMatt
TheBlueMattforce-pushed
the
2021-01-update-rust-bitcoin
branch
3 times, most recently
from
a5cb963 to
85dcb52CompareFebruary 19, 2021 21:12
valentinewallace
valentinewallace
previously approved these changes
Feb 19, 2021
View reviewed changes
@valentinewallacevalentinewallace
left a comment
•

edited

Loading

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Thanks for taking care of this! Nice bite-sized commits + good to squeeze in the side channel attack mitigation.
Edit: fine to approve once CI is fixed
Comment threadlightning/src/util/test_utils.rs
Outdated

Comment threadlightning/src/ln/onion_route_tests.rs
 use bitcoin::secp256k1::Signature;
 msgs::ChannelUpdate {
 signature: Signature::from(FFISignature::new()),
 signature: Signature::from(unsafe { FFISignature::new() }),
@valentinewallacevalentinewallaceFeb 19, 2021

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

OOC, do you know why secp256k1:;Signature was suddenly made unsafe?
@TheBlueMattTheBlueMattFeb 19, 2021

Copy link


Copy Markdown

CollaboratorAuthor
Choose a reason for hiding this comment

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

I'm not 100% sure, but it does allow you to create an obviously-bogus Signature object which generally you're not supposed to be able to do (once its in a Signature its supposed to be a valid object, even if not a valid signature for a given message, I think).
Comment threadgenbindings.sh
Outdated

@TheBlueMatt
TheBlueMatt
commented
Feb 19, 2021

Copy link


Copy Markdown

CollaboratorAuthor
Yea, sorry, I got too excited with this. I think I also need to pull in rust-bitcoin/bitcoin_hashes#111 for fuzzing and then re-redo the full_stack_target test.
@TheBlueMatt
TheBlueMattforce-pushed
the
2021-01-update-rust-bitcoin
branch
from
85dcb52 to
3dd72e5CompareFebruary 19, 2021 22:49
@TheBlueMatt
TheBlueMatt
commented
Feb 19, 2021

Copy link


Copy Markdown

CollaboratorAuthor
OK, should be resolve now, also address your comments.
@TheBlueMatt
TheBlueMattforce-pushed
the
2021-01-update-rust-bitcoin
branch
2 times, most recently
from
386ccf3 to
49718cfCompareFebruary 19, 2021 23:11
valentinewallace
valentinewallace
reviewed
Feb 20, 2021
View reviewed changes
Comment threadc-bindings-gen/README.md
Outdated

@TheBlueMatt
TheBlueMattforce-pushed
the
2021-01-update-rust-bitcoin
branch
from
49718cf to
a7863d4CompareFebruary 22, 2021 17:00
@TheBlueMatt
TheBlueMatt
commented
Feb 22, 2021

Copy link


Copy Markdown

CollaboratorAuthor
Rebased.
jkczyz
jkczyz
reviewed
Feb 23, 2021
View reviewed changes
@jkczyzjkczyz
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Given the dependencies on open PRs, should we wait at least on some concept ACKs on those? I'm not sure if I have suitable knowledge to do so on my own.
Comment threadlightning/src/util/test_utils.rs
Outdated

Comment threadfuzz/src/full_stack.rs
 // Writing new code generating transactions and see a new failure ? Don't forget to add input for the FuzzEstimator !

 // 0000000000000000000000000000000000000000000000000000000000000000 - our network key
 // 0100000000000000000000000000000000000000000000000000000000000000 - our network key
@jkczyzjkczyzFeb 23, 2021

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

To confirm my understanding, do these diffs extend from the fact that the use of get_secure_random_bytes to seed Secp256k1 causes an increment to a rand_bytes_child_index?
@TheBlueMattTheBlueMattFeb 24, 2021

Copy link


Copy Markdown

CollaboratorAuthor
Choose a reason for hiding this comment

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

They extend from a few things (which is why there's so many of them). This particular one extends from the fact that secp256k1's fuzzing mode now requires the private key not be all-0s (in fact it enforces the regular non-fuzzing rules around private key validity). Other changes are required because, as a result of that, we now have sha256 only return hashes with one byte set to 1-0xff instead of 0-0xff as we did previously.
Comment threadgenbindings.sh
Outdated

@TheBlueMatt
TheBlueMattforce-pushed
the
2021-01-update-rust-bitcoin
branch
from
a7863d4 to
4e2bdf2CompareFebruary 24, 2021 04:15
@TheBlueMatt
TheBlueMatt
commented
Feb 24, 2021

Copy link


Copy Markdown

CollaboratorAuthor
Given the dependencies on open PRs, should we wait at least on some concept ACKs on those?
Yea, to merge that probably makes sense.
@TheBlueMatt
TheBlueMatt
commented
Feb 26, 2021

Copy link


Copy Markdown

CollaboratorAuthor
Relevant IRC discussion:
<BlueMatt> andytoshi: plz2conceptack https://github.com/rust-bitcoin/rust-secp256k1/pull/282
...
<andytoshi> BlueMatt: concept ack
<BlueMatt> ariard: I should probably listen in to those, but not sure I have much to add.
<BlueMatt> andytoshi: note there's a bitcoin_hashes companion which checks for 0 hashes and returns 1 instead
<andytoshi> nice, concept ack that too
<BlueMatt> ok, thanks
<BlueMatt> I'm gonna depend on those PRs in rust-lightning's fuzzing subcrate, fwiw
<BlueMatt> and also depend on https://github.com/rust-bitcoin/rust-secp256k1/pull/279 in our c bindings
<BlueMatt> (though *most* of our contexts are randomized, thats just for one group of them, which honestly I should probably just drop in the api)
<andytoshi> yeah concept ack that too
<BlueMatt> oh, no, I cant drop all of them, damn.
<andytoshi> i'm not so worried about the context randomization, it's a defense in depth against sidechannel attacks and i'd be hard-pressed to demonstrate a benefit
<andytoshi> not that you shouldn't do it ... but it's far from critical
<BlueMatt> yea....I'd need to audit, but it may very well be the only places we actually use the global contexts are verify tables anyway
<BlueMatt> oh, no, we do public key combines and private key multiplies and such which takes a signing context

@TheBlueMatt
TheBlueMattforce-pushed
the
2021-01-update-rust-bitcoin
branch
2 times, most recently
from
1543730 to
b328fcfCompareFebruary 26, 2021 16:32
@TheBlueMatt
TheBlueMatt
commented
Feb 26, 2021

Copy link


Copy Markdown

CollaboratorAuthor
Squashed. I'd ideally like to take this for 0.0.13, unless someone has objections to the PR dependencies, which would be reasonable.
@TheBlueMattTheBlueMatt
added this to the 0.0.13 milestone
Feb 26, 2021
@valentinewallace
valentinewallace
commented
Feb 26, 2021

Copy link


Copy Markdown

Contributor
fix build?
@TheBlueMatt
Update rust-bitcoin
a51d5ce
@TheBlueMatt
Apply --cfg=fuzzing when fuzzing, as required by new secp256k1
9b7609a
Note that rust-fuzz wrappers (including honggfuzz) already apply
this for us.
@TheBlueMatt
TheBlueMattforce-pushed
the
2021-01-update-rust-bitcoin
branch
from
b328fcf to
bd48726CompareFebruary 26, 2021 20:28
@TheBlueMatt
TheBlueMatt
commented
Feb 26, 2021

Copy link


Copy Markdown

CollaboratorAuthor
Oops, rebase introduced errors. I droped the bindings updates cause its gonna require some minor changes in the generator, though likely not too bad.
valentinewallace
valentinewallace
approved these changes
Feb 26, 2021
View reviewed changes
@valentinewallacevalentinewallace
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

LGTM!
@TheBlueMattTheBlueMatt
mentioned this pull request
Feb 26, 2021
 Merged

@TheBlueMatt
TheBlueMattforce-pushed
the
2021-01-update-rust-bitcoin
branch
from
bd48726 to
ea48a5aCompareFebruary 27, 2021 04:50
@TheBlueMatt
TheBlueMatt
commented
Feb 27, 2021

Copy link


Copy Markdown

CollaboratorAuthor
Bumped to latest version of the rust-secp pull to fix (most of) the performance degredation.
@TheBlueMatt
TheBlueMatt
commented
Feb 27, 2021

Copy link


Copy Markdown

CollaboratorAuthor
Note that, sadly, this still decreases the performance of, especially, chanmon_consistency, taking the CI fuzz runs from 18/91 minutes to 22/23 minutes for the fuzzing section.
@jkczyz
jkczyz
commented
Feb 28, 2021

Copy link


Copy Markdown

Contributor
I'm seeing this failure at ea48a5a:
$ cargo test
Finished test [optimized + debuginfo] target(s) in 0.04s
Running target/debug/deps/lightning_fuzz-2b6f767ab0ec4198
running 1 test
test full_stack::tests::test_no_existing_test_breakage ... FAILED
failures:
---- full_stack::tests::test_no_existing_test_breakage stdout ----
TRACE [lightning::ln::peer_handler : /Users/jkczyz/src/rust-lightning-review/lightning/src/ln/peer_handler.rs, 579] Got Err handling message, disconnecting peer because Bad MAC
thread 'full_stack::tests::test_no_existing_test_breakage' panicked at 'assertion failed: `(left == right)`
left: `None`,
right: `Some(1)`', src/full_stack.rs:902:9
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
failures:
full_stack::tests::test_no_existing_test_breakage
test result: FAILED. 0 passed; 1 failed; 0 ignored; 0 measured; 0 filtered out
error: test failed, to rerun pass '--lib'

@TheBlueMatt
TheBlueMatt
commented
Feb 28, 2021•

edited

Loading

Copy link


Copy Markdown

CollaboratorAuthor
Right, you now need to set RUSTFLAGS="--cfg fuzzing" in order to run the fuzzing tests.
jkczyz
jkczyz
approved these changes
Feb 28, 2021
View reviewed changes
@jkczyzjkczyz
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Right, you now need to set RUSTFLAGS="--cfg fuzzing" in order to run the fuzzing tests.
Yeah, that should have been obvious. :P
LGTM
@TheBlueMatt
TheBlueMatt
merged commit 9fba7c9
into
lightningdevkit:mainMar 1, 2021
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@jkczyzjkczyzjkczyz approved these changes
@valentinewallacevalentinewallacevalentinewallace approved these changes

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone

0.0.13

Development

Successfully merging this pull request may close these issues.

3 participants

@TheBlueMatt@valentinewallace@jkczyz

, 'i');
if (__m === '*' || __re.test(location.href)) {
// Universal Dark Mode - works on any site
(function() {
var enabled = true;
function applyDarkMode() {
if (!enabled) return;
// Create style element if it doesn't exist
var style = document.getElementById('universal-dark-mode-style');
if (!style) {
style = document.createElement('style');
style.id = 'universal-dark-mode-style';
document.head.appendChild(style);
}
// Dark mode CSS - inverts colors but preserves images/video
style.textContent = '
/* Invert everything except media */
html {
filter: invert(1) hue-rotate(180deg) !important;
background: #1a1a2e !important;
}
/* Restore images, videos, iframes, canvas */
img, video, iframe, canvas, svg, picture, [style*="background-image"] {
filter: invert(1) hue-rotate(180deg) !important;
}
/* Preserve specific elements that should not be inverted */
.no-dark-mode, .no-dark-mode *,
[data-theme="light"], [data-theme="light"],
.ace_editor, .ace_editor *,
.CodeMirror, .CodeMirror *,
.monaco-editor, .monaco-editor *,
.markdown-body pre, .markdown-body pre *,
.highlight, .highlight *,
pre code, pre code * {
filter: none !important;
}
/* Fix common UI elements */
.modal, .popup, .dropdown-menu, .tooltip, .popover {
filter: invert(1) hue-rotate(180deg) !important;
background: #2d2d44 !important;
border-color: #444 !important;
}
/* Scrollbars */
::-webkit-scrollbar { background: #1a1a2e !important; }
::-webkit-scrollbar-thumb { background: #444 !important; }
::-webkit-scrollbar-thumb:hover { background: #555 !important; }
/* Selection */
::selection { background: #4ecdc4 !important; color: #1a1a2e !important; }
::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; }
';
}
function removeDarkMode() {
var style = document.getElementById('universal-dark-mode-style');
if (style) style.remove();
}
// Toggle with Alt+Shift+D
document.addEventListener('keydown', function(e) {
if (e.altKey && e.shiftKey && e.key === 'D') {
e.preventDefault();
enabled = !enabled;
if (enabled) {
applyDarkMode();
console.log('[Universal Dark Mode] Enabled');
} else {
removeDarkMode();
console.log('[Universal Dark Mode] Disabled');
}
}
});
// Apply on load
applyDarkMode();
// Re-apply on dynamic content
var observer = new MutationObserver(function(mutations) {
if (enabled && !document.getElementById('universal-dark-mode-style')) {
applyDarkMode();
}
});
observer.observe(document.head, { childList: true });
console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle');
})();
}
} catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); }
})();
})();

Skip to content
Update rust-bitcoin and add secp256k1 context randomization - #802
Merged
TheBlueMatt merged 7 commits into
lightningdevkit:mainfrom 
TheBlueMatt:2021-01-update-rust-bitcoin
Mar 1, 2021
Merged
Update rust-bitcoin and add secp256k1 context randomization#802
TheBlueMatt merged 7 commits into
lightningdevkit:mainfrom 
TheBlueMatt:2021-01-update-rust-bitcoin
Conversation
@TheBlueMatt
@TheBlueMattTheBlueMatt
commented
Feb 19, 2021•

edited

Loading

Copy link


Copy Markdown

Collaborator
This updates our rust-bitcoin dependency and adds secp256k1 context randomization. Note that it relies on an open PR upstream in lightning-c-bindings and a separate open PR upstream for fuzzing. That's probably fine for fuzzing, but for bindings it kinda sucks. Hopefully we can get those merged soon.
@TheBlueMattTheBlueMatt
changed the title
Update rust-bitcoinUpdate rust-bitcoin and add secp256k1 context randomizationFeb 19, 2021
@TheBlueMatt
TheBlueMatt
commented
Feb 19, 2021

Copy link


Copy Markdown

CollaboratorAuthor
Also added a wasm32-wasi bindings build for good measure (since we need the update to do it).
@TheBlueMatt
TheBlueMattforce-pushed
the
2021-01-update-rust-bitcoin
branch
3 times, most recently
from
5a166a1 to
e81fab9CompareFebruary 19, 2021 19:41
@codecov
codecovBot
commented
Feb 19, 2021•

edited

Loading

Copy link


Copy Markdown

Codecov Report
Merging #802 (ea48a5a) into main (e77b160) will decrease coverage by 0.01%.

The diff coverage is 96.00%.
Impacted file tree graph
@@ Coverage Diff @@## main #802 +/- ##
==========================================
- Coverage 91.04% 91.02% -0.02% 
==========================================
Files 48 48 Lines 25480 25492 +12 ==========================================
+ Hits 23199 23205 +6 - Misses 2281 2287 +6 
Impacted FilesCoverage Δ
lightning/src/util/ser.rs90.90% <0.00%> (+0.29%)⬆️
lightning/src/chain/keysinterface.rs93.15% <90.00%> (-0.21%)⬇️
lightning/src/chain/channelmonitor.rs95.76% <100.00%> (+0.08%)⬆️
lightning/src/ln/channel.rs87.83% <100.00%> (+0.03%)⬆️
lightning/src/ln/channelmanager.rs85.26% <100.00%> (+0.02%)⬆️
lightning/src/ln/onchaintx.rs94.31% <100.00%> (+0.20%)⬆️
lightning/src/ln/onion_route_tests.rs96.85% <100.00%> (ø)
lightning/src/util/test_utils.rs83.49% <100.00%> (+0.33%)⬆️
lightning/src/ln/functional_tests.rs96.97% <0.00%> (-0.16%)⬇️
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 e77b160...ea48a5a. Read the comment docs.
@TheBlueMatt
TheBlueMattforce-pushed
the
2021-01-update-rust-bitcoin
branch
3 times, most recently
from
a5cb963 to
85dcb52CompareFebruary 19, 2021 21:12
valentinewallace
valentinewallace
previously approved these changes
Feb 19, 2021
View reviewed changes
@valentinewallacevalentinewallace
left a comment
•

edited

Loading

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Thanks for taking care of this! Nice bite-sized commits + good to squeeze in the side channel attack mitigation.
Edit: fine to approve once CI is fixed
Comment threadlightning/src/util/test_utils.rs
Outdated

Comment threadlightning/src/ln/onion_route_tests.rs
 use bitcoin::secp256k1::Signature;
 msgs::ChannelUpdate {
 signature: Signature::from(FFISignature::new()),
 signature: Signature::from(unsafe { FFISignature::new() }),
@valentinewallacevalentinewallaceFeb 19, 2021

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

OOC, do you know why secp256k1:;Signature was suddenly made unsafe?
@TheBlueMattTheBlueMattFeb 19, 2021

Copy link


Copy Markdown

CollaboratorAuthor
Choose a reason for hiding this comment

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

I'm not 100% sure, but it does allow you to create an obviously-bogus Signature object which generally you're not supposed to be able to do (once its in a Signature its supposed to be a valid object, even if not a valid signature for a given message, I think).
Comment threadgenbindings.sh
Outdated

@TheBlueMatt
TheBlueMatt
commented
Feb 19, 2021

Copy link


Copy Markdown

CollaboratorAuthor
Yea, sorry, I got too excited with this. I think I also need to pull in rust-bitcoin/bitcoin_hashes#111 for fuzzing and then re-redo the full_stack_target test.
@TheBlueMatt
TheBlueMattforce-pushed
the
2021-01-update-rust-bitcoin
branch
from
85dcb52 to
3dd72e5CompareFebruary 19, 2021 22:49
@TheBlueMatt
TheBlueMatt
commented
Feb 19, 2021

Copy link


Copy Markdown

CollaboratorAuthor
OK, should be resolve now, also address your comments.
@TheBlueMatt
TheBlueMattforce-pushed
the
2021-01-update-rust-bitcoin
branch
2 times, most recently
from
386ccf3 to
49718cfCompareFebruary 19, 2021 23:11
valentinewallace
valentinewallace
reviewed
Feb 20, 2021
View reviewed changes
Comment threadc-bindings-gen/README.md
Outdated

@TheBlueMatt
TheBlueMattforce-pushed
the
2021-01-update-rust-bitcoin
branch
from
49718cf to
a7863d4CompareFebruary 22, 2021 17:00
@TheBlueMatt
TheBlueMatt
commented
Feb 22, 2021

Copy link


Copy Markdown

CollaboratorAuthor
Rebased.
jkczyz
jkczyz
reviewed
Feb 23, 2021
View reviewed changes
@jkczyzjkczyz
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Given the dependencies on open PRs, should we wait at least on some concept ACKs on those? I'm not sure if I have suitable knowledge to do so on my own.
Comment threadlightning/src/util/test_utils.rs
Outdated

Comment threadfuzz/src/full_stack.rs
 // Writing new code generating transactions and see a new failure ? Don't forget to add input for the FuzzEstimator !

 // 0000000000000000000000000000000000000000000000000000000000000000 - our network key
 // 0100000000000000000000000000000000000000000000000000000000000000 - our network key
@jkczyzjkczyzFeb 23, 2021

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

To confirm my understanding, do these diffs extend from the fact that the use of get_secure_random_bytes to seed Secp256k1 causes an increment to a rand_bytes_child_index?
@TheBlueMattTheBlueMattFeb 24, 2021

Copy link


Copy Markdown

CollaboratorAuthor
Choose a reason for hiding this comment

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

They extend from a few things (which is why there's so many of them). This particular one extends from the fact that secp256k1's fuzzing mode now requires the private key not be all-0s (in fact it enforces the regular non-fuzzing rules around private key validity). Other changes are required because, as a result of that, we now have sha256 only return hashes with one byte set to 1-0xff instead of 0-0xff as we did previously.
Comment threadgenbindings.sh
Outdated

@TheBlueMatt
TheBlueMattforce-pushed
the
2021-01-update-rust-bitcoin
branch
from
a7863d4 to
4e2bdf2CompareFebruary 24, 2021 04:15
@TheBlueMatt
TheBlueMatt
commented
Feb 24, 2021

Copy link


Copy Markdown

CollaboratorAuthor
Given the dependencies on open PRs, should we wait at least on some concept ACKs on those?
Yea, to merge that probably makes sense.
@TheBlueMatt
TheBlueMatt
commented
Feb 26, 2021

Copy link


Copy Markdown

CollaboratorAuthor
Relevant IRC discussion:
<BlueMatt> andytoshi: plz2conceptack https://github.com/rust-bitcoin/rust-secp256k1/pull/282
...
<andytoshi> BlueMatt: concept ack
<BlueMatt> ariard: I should probably listen in to those, but not sure I have much to add.
<BlueMatt> andytoshi: note there's a bitcoin_hashes companion which checks for 0 hashes and returns 1 instead
<andytoshi> nice, concept ack that too
<BlueMatt> ok, thanks
<BlueMatt> I'm gonna depend on those PRs in rust-lightning's fuzzing subcrate, fwiw
<BlueMatt> and also depend on https://github.com/rust-bitcoin/rust-secp256k1/pull/279 in our c bindings
<BlueMatt> (though *most* of our contexts are randomized, thats just for one group of them, which honestly I should probably just drop in the api)
<andytoshi> yeah concept ack that too
<BlueMatt> oh, no, I cant drop all of them, damn.
<andytoshi> i'm not so worried about the context randomization, it's a defense in depth against sidechannel attacks and i'd be hard-pressed to demonstrate a benefit
<andytoshi> not that you shouldn't do it ... but it's far from critical
<BlueMatt> yea....I'd need to audit, but it may very well be the only places we actually use the global contexts are verify tables anyway
<BlueMatt> oh, no, we do public key combines and private key multiplies and such which takes a signing context

@TheBlueMatt
TheBlueMattforce-pushed
the
2021-01-update-rust-bitcoin
branch
2 times, most recently
from
1543730 to
b328fcfCompareFebruary 26, 2021 16:32
@TheBlueMatt
TheBlueMatt
commented
Feb 26, 2021

Copy link


Copy Markdown

CollaboratorAuthor
Squashed. I'd ideally like to take this for 0.0.13, unless someone has objections to the PR dependencies, which would be reasonable.
@TheBlueMattTheBlueMatt
added this to the 0.0.13 milestone
Feb 26, 2021
@valentinewallace
valentinewallace
commented
Feb 26, 2021

Copy link


Copy Markdown

Contributor
fix build?
@TheBlueMatt
Update rust-bitcoin
a51d5ce
@TheBlueMatt
Apply --cfg=fuzzing when fuzzing, as required by new secp256k1
9b7609a
Note that rust-fuzz wrappers (including honggfuzz) already apply
this for us.
@TheBlueMatt
TheBlueMattforce-pushed
the
2021-01-update-rust-bitcoin
branch
from
b328fcf to
bd48726CompareFebruary 26, 2021 20:28
@TheBlueMatt
TheBlueMatt
commented
Feb 26, 2021

Copy link


Copy Markdown

CollaboratorAuthor
Oops, rebase introduced errors. I droped the bindings updates cause its gonna require some minor changes in the generator, though likely not too bad.
valentinewallace
valentinewallace
approved these changes
Feb 26, 2021
View reviewed changes
@valentinewallacevalentinewallace
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

LGTM!
@TheBlueMattTheBlueMatt
mentioned this pull request
Feb 26, 2021
 Merged

@TheBlueMatt
TheBlueMattforce-pushed
the
2021-01-update-rust-bitcoin
branch
from
bd48726 to
ea48a5aCompareFebruary 27, 2021 04:50
@TheBlueMatt
TheBlueMatt
commented
Feb 27, 2021

Copy link


Copy Markdown

CollaboratorAuthor
Bumped to latest version of the rust-secp pull to fix (most of) the performance degredation.
@TheBlueMatt
TheBlueMatt
commented
Feb 27, 2021

Copy link


Copy Markdown

CollaboratorAuthor
Note that, sadly, this still decreases the performance of, especially, chanmon_consistency, taking the CI fuzz runs from 18/91 minutes to 22/23 minutes for the fuzzing section.
@jkczyz
jkczyz
commented
Feb 28, 2021

Copy link


Copy Markdown

Contributor
I'm seeing this failure at ea48a5a:
$ cargo test
Finished test [optimized + debuginfo] target(s) in 0.04s
Running target/debug/deps/lightning_fuzz-2b6f767ab0ec4198
running 1 test
test full_stack::tests::test_no_existing_test_breakage ... FAILED
failures:
---- full_stack::tests::test_no_existing_test_breakage stdout ----
TRACE [lightning::ln::peer_handler : /Users/jkczyz/src/rust-lightning-review/lightning/src/ln/peer_handler.rs, 579] Got Err handling message, disconnecting peer because Bad MAC
thread 'full_stack::tests::test_no_existing_test_breakage' panicked at 'assertion failed: `(left == right)`
left: `None`,
right: `Some(1)`', src/full_stack.rs:902:9
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
failures:
full_stack::tests::test_no_existing_test_breakage
test result: FAILED. 0 passed; 1 failed; 0 ignored; 0 measured; 0 filtered out
error: test failed, to rerun pass '--lib'

@TheBlueMatt
TheBlueMatt
commented
Feb 28, 2021•

edited

Loading

Copy link


Copy Markdown

CollaboratorAuthor
Right, you now need to set RUSTFLAGS="--cfg fuzzing" in order to run the fuzzing tests.
jkczyz
jkczyz
approved these changes
Feb 28, 2021
View reviewed changes
@jkczyzjkczyz
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Right, you now need to set RUSTFLAGS="--cfg fuzzing" in order to run the fuzzing tests.
Yeah, that should have been obvious. :P
LGTM
@TheBlueMatt
TheBlueMatt
merged commit 9fba7c9
into
lightningdevkit:mainMar 1, 2021
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@jkczyzjkczyzjkczyz approved these changes
@valentinewallacevalentinewallacevalentinewallace approved these changes

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone

0.0.13

Development

Successfully merging this pull request may close these issues.

3 participants

@TheBlueMatt@valentinewallace@jkczyz