Pad ohttp req/res messages to consistent 8192 bytes #395

DanGould · 2024-11-20T02:24:03Z

Padded messages (POST, GET, Response 202, Response 200) are no longer distinguishable

Rationale: 7kb x 2x/min / 1h = ~1MB/hr which is not too much overhead.

~~We must also specify a 30s timeout in BIP 77 for long polling and say that clients MUST poll as soon as they receive a response so as not to fingerprint clients based on request timing.~~ TODO left in BIP comments
allocate fixed length arrays instead of resizeing vectors.

Clients pad OHTTP requests and responses so that when they're sent to an OHTTP relay that relay can't distingush the type of BIP 77 message e.g. POST, GET, Response 202, Response 200. Co-authored-by: nothingmuch <nothingmuch@woobling.org>

DanGould · 2024-11-23T18:39:22Z

If these arguments were statically sized we wouldn't need to test it. Do that

spacebear21

concept ACK

payjoin-directory/src/lib.rs

payjoin/src/send/mod.rs

coveralls · 2024-11-27T17:34:28Z

Pull Request Test Coverage Report for Build 12063584579

Details

35 of 53 (66.04%) changed or added relevant lines in 8 files are covered.
2 unchanged lines in 1 file lost coverage.
Overall coverage decreased (-0.1%) to 61.494%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
payjoin/src/send/error.rs	0	2	0.0%
payjoin-cli/src/app/v2.rs	0	3	0.0%
payjoin/src/receive/v2/mod.rs	14	20	70.0%
payjoin/src/receive/v2/error.rs	0	7	0.0%

Files with Coverage Reduction	New Missed Lines	%
payjoin/src/send/mod.rs	2	92.78%

Totals
Change from base Build 12037630831:	-0.1%
Covered Lines:	2790
Relevant Lines:	4537

💛 - Coveralls

spacebear21 · 2024-11-27T23:57:37Z

payjoin/src/send/mod.rs

+        let mut res_buf = [0u8; crate::ohttp::ENCAPSULATED_MESSAGE_BYTES];
+        res_buf[..response.len()].copy_from_slice(response);
        let response = ohttp_decapsulate(self.ohttp_ctx, &res_buf)


Following the same pattern as V2GetContext::process_response:

Suggested change

let mut res_buf = [0u8; crate::ohttp::ENCAPSULATED_MESSAGE_BYTES];

res_buf[..response.len()].copy_from_slice(response);

let response = ohttp_decapsulate(self.ohttp_ctx, &res_buf)

let response_array: &[u8; crate::ohttp::ENCAPSULATED_MESSAGE_BYTES] =

response

.try_into()

.map_err(|_| InternalValidationError::UnexpectedResponseSize(response.len()))?;

let response = ohttp_decapsulate(ohttp_ctx, response_array)

Also seems better to propagate an error instead of truncating to response.len() in case the length doesn't match the expected size.

this is important, because if response.len() > crate::ohttp::ENCAPSULATED_MESSAGE_BYTES then this will panic due to slice out of range crashing the directory

nothingmuch

i selected "Request changes" because spacebear's suggestion is required (addresses out of range panic in directory). my suggestions are mere comments.

nothingmuch · 2024-11-28T02:55:29Z

payjoin/src/ohttp.rs

+
+    let mut bhttp_req = [0u8; PADDED_BHTTP_REQ_BYTES];
+    let mut cursor = std::io::Cursor::new(&mut bhttp_req[..]);
+    let _ = bhttp_message.write_bhttp(bhttp::Mode::KnownLength, &mut cursor);


IIRC wrapping with Cursor is not necessary as a mut slice implements Write (not sure if write_bhttp needs Seek?)

Ah, we need .as_mut_slice. We kept missing this method when we stayed up late trying to fix it.

nothingmuch · 2024-11-28T03:00:52Z

payjoin/src/send/mod.rs

+        let mut res_buf = [0u8; crate::ohttp::ENCAPSULATED_MESSAGE_BYTES];
+        res_buf[..response.len()].copy_from_slice(response);
        let response = ohttp_decapsulate(self.ohttp_ctx, &res_buf)


this is important, because if response.len() > crate::ohttp::ENCAPSULATED_MESSAGE_BYTES then this will panic due to slice out of range crashing the directory

nothingmuch · 2024-11-28T03:03:10Z

payjoin-directory/src/lib.rs

@@ -23,7 +23,8 @@ pub const DEFAULT_DIR_PORT: u16 = 8080;
 pub const DEFAULT_DB_HOST: &str = "localhost:6379";
 pub const DEFAULT_TIMEOUT_SECS: u64 = 30;

-const MAX_BUFFER_SIZE: usize = 65536;
+const PADDED_BHTTP_BYTES: usize = 8192;


we should probably document the total waste (8192 - ohttp overhead - 7168) somewhere, but not sure where is a good place to do that

I was wondering about this. Can we match them to create no or less waste? my understanding is that we just need leeway for sufficiently long URLs in headers

when we tried to account for the bhttp overhead we got lost in details and specifically how it varies... i'm not sure what we can promise there

my framing it as "waste" was misleading, so we should come up with a way of explaining this gap in sizes more clearly.

the bhttp encoding overhead, any variability in the vhost, uri path, etc must all fit in this ~1kib budget... that seems reasonable as a value, i'm not sure if we should match these more closely although i can only think of pretty contrived reasons why you'd that much

path in an OHTTP request is in practice quite small since the target path details are encapsulated and the request ought to go to the OHTTP gateway to be marshaled to the specific path target encoded in the BHTTP. In our examples, we just use / as the path, and the host https://payjo.in'. https://payjo.in/` is 17 bytes. That makes me think this overhead is actually a lot more than it needs to be and 256 bytes, being an order of magnitude greater, is almost certainly enough for any legitimate use assuming BIP 77 is the primary use. Getting 768 additional bytes if we used 256 bytes in the payload is an ~11% increase in payload size, which is a pretty big gain for working with PSBTs for payjoins.

i thought 7168 seemed and was deemed sufficient for the 2 party case? has anything changed the underlying rationale for that? FWIW a similar% gain would be obtained by not encoding the psbt as base64 in v2

Take advantage of the edit to use `&[u8]` function signatures where applicable to reduce tech debt.

nothingmuch

This seems mergeable to me as is.

However, if there are good reasons to doubt 7168 * 6/8 = ~5KiB PSBT as being sufficiently large, or 8192 as being sufficiently small then it should probably be addressed.

Both seem rather arbitrary to me, but ~5KiB being sufficiently large for BIP 77 is the more important one to get right i guess? A budget of about a dozen each of inputs and outputs per party with room to spare seems like it's more than enough for the stated use cases.

The only argument I can think of that might make a difference in user experience -- and even that is very dubious -- depends on the actual MTU. The lowest value on wikipedia for common media is 1452, which * 5 = 7260. For one packet to make a perceptible difference on average would imply the user is suffering from pretty extreme packet loss. Assuming this argument actually makes sense (again I think it doesn't) then i guess we should calculate and verify precise values for the bhttp overhead etc to make sure it all fits in less than 7260 bytes. Otherwise the argument for utilizing the existing headroom in 8192 bytes more efficiently seems very marginal for the 2 party case.

DanGould · 2024-11-29T13:56:01Z

This seems mergeable to me as is

You sound very confident that this will not be followed with a breaking change. Merging.

nothingmuch · 2024-11-29T18:49:43Z

You sound very confident that this will not be followed with a breaking change. Merging.

Merely very confident that I can't come up with a good argument for increasing or decreasing either of the two sizes ;-)

DanGould force-pushed the uniform-ohttp-payloads branch 2 times, most recently from dedec61 to b404a91 Compare November 20, 2024 02:26

DanGould force-pushed the uniform-ohttp-payloads branch from ac0f294 to 1a9179b Compare November 26, 2024 18:19

DanGould requested review from nothingmuch and spacebear21 November 26, 2024 18:26

spacebear21 reviewed Nov 27, 2024

View reviewed changes

payjoin-directory/src/lib.rs Outdated Show resolved Hide resolved

payjoin/src/send/mod.rs Outdated Show resolved Hide resolved

DanGould force-pushed the uniform-ohttp-payloads branch from 1a9179b to d6f927c Compare November 27, 2024 17:29

spacebear21 reviewed Nov 27, 2024

View reviewed changes

nothingmuch requested changes Nov 28, 2024

View reviewed changes

DanGould mentioned this pull request Nov 28, 2024

Expose new shared payjoin::directory types #418

Open

Pass static size ohttp en/decapsulate arguments

c031be7

Take advantage of the edit to use `&[u8]` function signatures where applicable to reduce tech debt.

DanGould force-pushed the uniform-ohttp-payloads branch from d6f927c to c031be7 Compare November 28, 2024 06:19

nothingmuch approved these changes Nov 28, 2024

View reviewed changes

DanGould merged commit 5812c84 into payjoin:master Nov 29, 2024
6 checks passed

DanGould deleted the uniform-ohttp-payloads branch November 29, 2024 13:56

spacebear21 mentioned this pull request Nov 29, 2024

Make statically sized array return types according to the spec #404

Closed

DanGould mentioned this pull request Dec 3, 2024

Release 0.21.0 #390

Open

17 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Pad ohttp req/res messages to consistent 8192 bytes #395

Pad ohttp req/res messages to consistent 8192 bytes #395

DanGould commented Nov 20, 2024 •

edited

Loading

DanGould commented Nov 23, 2024

spacebear21 left a comment

coveralls commented Nov 27, 2024 •

edited

Loading

spacebear21 Nov 27, 2024 •

edited

Loading

nothingmuch Nov 28, 2024

nothingmuch left a comment

nothingmuch Nov 28, 2024

DanGould Nov 28, 2024

nothingmuch Nov 28, 2024

nothingmuch Nov 28, 2024

DanGould Nov 28, 2024

nothingmuch Nov 28, 2024

DanGould Nov 28, 2024 •

edited

Loading

nothingmuch Nov 28, 2024

nothingmuch left a comment •

edited

Loading

DanGould commented Nov 29, 2024

nothingmuch commented Nov 29, 2024

Pad ohttp req/res messages to consistent 8192 bytes #395

Pad ohttp req/res messages to consistent 8192 bytes #395

Conversation

DanGould commented Nov 20, 2024 • edited Loading

DanGould commented Nov 23, 2024

spacebear21 left a comment

Choose a reason for hiding this comment

coveralls commented Nov 27, 2024 • edited Loading

Pull Request Test Coverage Report for Build 12063584579

Details

💛 - Coveralls

spacebear21 Nov 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nothingmuch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DanGould Nov 28, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nothingmuch left a comment • edited Loading

Choose a reason for hiding this comment

DanGould commented Nov 29, 2024

nothingmuch commented Nov 29, 2024

DanGould commented Nov 20, 2024 •

edited

Loading

coveralls commented Nov 27, 2024 •

edited

Loading

spacebear21 Nov 27, 2024 •

edited

Loading

DanGould Nov 28, 2024 •

edited

Loading

nothingmuch left a comment •

edited

Loading