Bitcoin Core Github
44 subscribers
121K links
Download Telegram
🤔 fjahr reviewed a pull request: "index: Deduplicate HashKey / HeightKey handling"
(https://github.com/bitcoin/bitcoin/pull/32997#pullrequestreview-3034913148)
Looks good, aside from the documentation cleanup in `blockfilterindex.cpp` you can consider my comments optional.
💬 fjahr commented on pull request "index: Deduplicate HashKey / HeightKey handling":
(https://github.com/bitcoin/bitcoin/pull/32997#discussion_r2216989959)
The comment above here still talks about three items stored for each block but after the change only one of the values is here. This would need to be adopted somehow and probably some of the docs moved over to `index/key.h`.

EDIT: Maybe the current comment can be left almost untouched but there needs to be a reference to where the keys are now.
💬 fjahr commented on pull request "index: Deduplicate HashKey / HeightKey handling":
(https://github.com/bitcoin/bitcoin/pull/32997#discussion_r2216982231)
nit: could do IWYU I guess, even though this wasn't followed that well in the index cpp files.

```
#include <dbwrapper.h>
#include <interfaces/types.h>
```
💬 fjahr commented on pull request "index: Deduplicate HashKey / HeightKey handling":
(https://github.com/bitcoin/bitcoin/pull/32997#discussion_r2216992529)
Also, very minor naming nit: File could be called `db_key.h` since we have other kinds of keys but I don't feel too strongly about it.
💬 fjahr commented on pull request "index: Deduplicate HashKey / HeightKey handling":
(https://github.com/bitcoin/bitcoin/pull/32997#discussion_r2216991906)
Aside from the documentation moving necessary mentioned above maybe there could be 1-2 general sentences here that this deals with the hash/height keys and that it's mostly relevant to indexes that use these.
💬 l0rinc commented on pull request "[IBD] multi-byte block obfuscation":
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2217023785)
Ok, will take @maflcko's suggestion if I have to push again.
💬 l0rinc commented on pull request "[IBD] multi-byte block obfuscation":
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2217028864)
Did that before, but @hodlinator pointed out that the previous version made more sense since it's storing obfuscation key's database key: https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2068663871.
💬 l0rinc commented on pull request "[IBD] multi-byte block obfuscation":
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2217024259)
> The simplest way to write this would be:
> m_obfuscation = Obfuscation{FastRandomContext{}.randbytes<Obfuscation::KEY_SIZE>()};
> Write(OBFUSCATION_KEY_KEY, m_obfuscation);

I don't think so, `Write` needs `m_obfuscation`, so it would obfuscate the key itself if we assign it before the write, try `build/bin/test_bitcoin --run_test=dbwrapper_tests`.
Seems the comment I added still isn't helping, how do I rephrase it to make this part obvious?

```C++
const Obfuscation new_key{FastRandom
...
💬 l0rinc commented on pull request "[IBD] multi-byte block obfuscation":
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2217039648)
Not sure I understand why this is better, we have the xor instructions spread to multiple places now - apply_random_xor_chunks & inline `original ^ key_bytes`. What do others think?
🤔 l0rinc reviewed a pull request: "[IBD] multi-byte block obfuscation"
(https://github.com/bitcoin/bitcoin/pull/31144#pullrequestreview-3034969188)
Thanks for the review.
Since the PR is in review for almost a year now, and since the remaining comments seem like they can potentially be done in follow-ups as well, I'd prefer getting this over the finish line soon.
💬 l0rinc commented on pull request "[IBD] multi-byte block obfuscation":
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2217034784)
Sure, if I push again, I'll consider these nits
💬 l0rinc commented on pull request "[IBD] multi-byte block obfuscation":
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2217030868)
If you insist, I'll do it if I have to push again.
💬 l0rinc commented on pull request "[IBD] multi-byte block obfuscation":
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2217033100)
Can you please point me to a documentation or code which demonstrates that?
💬 l0rinc commented on pull request "[IBD] multi-byte block obfuscation":
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2217040108)
Sure, I'll merge those commits next time I push
💬 l0rinc commented on pull request "mempool: Avoid needless vtx iteration during IBD":
(https://github.com/bitcoin/bitcoin/pull/32827#issuecomment-3091278861)
Had to rebase after https://github.com/bitcoin/bitcoin/commit/067365d2a8a421a074bb54394118beccb3f775c2#diff-e6100361fa0e9e25478f808ca084e5f681d4dddbbee7b3bea0f9d5bcd29db3aaR39-R563, `src/txorphanage.cpp` was moved to `src/node/txorphanage.cpp`, the change is exactly the same otherwise, would appreciate re-reviews.
💬 achow101 commented on pull request "[IBD] multi-byte block obfuscation":
(https://github.com/bitcoin/bitcoin/pull/31144#issuecomment-3091292177)
ACK 248b6a27c351690d3596711cc36b8102977adeab
💬 lifofifoX commented on pull request "Reduce minrelaytxfee to 100 sats/kvB":
(https://github.com/bitcoin/bitcoin/pull/32959#issuecomment-3091388547)
> A short while ago I sent an RBF with a 0.5 sat/vB fee, waited 30 minutes, and it still hadn’t reached mempool.space’s node

I believe that's because mempool.space nodes did not lower `incrementalrelayfee` through [config](https://github.com/mempool/mempool/blob/master/production/bitcoin.conf#L23C1-L23C14).
📝 b-l-u-e opened a pull request: "rpc: Fix internal bug in descriptorprocesspsbt when encountering invalid signatures"
(https://github.com/bitcoin/bitcoin/pull/33014)
## Summary
Fixes #32849 - Replace `CHECK_NONFATAL` with proper error handling in `descriptorprocesspsbt` RPC to prevent internal bug assertions when encountering invalid Schnorr signatures.

## Problem
When `descriptorprocesspsbt` encounters a PSBT with invalid signatures (e.g., invalid Schnorr signatures with SIGHASH_SINGLE | ANYONECANPAY flags), it triggers an internal bug assertion instead of returning a user-friendly error message.

## Solution
Replace the `CHECK_NONFATAL(FinalizeAndE
...
🚀 achow101 merged a pull request: "[IBD] multi-byte block obfuscation"
(https://github.com/bitcoin/bitcoin/pull/31144)
💬 ajtowns commented on pull request "Bump SCRIPT_VERIFY flags to 64 bit":
(https://github.com/bitcoin/bitcoin/pull/32998#issuecomment-3091891676)
> Minified this is:

I don't think you need the `git fetch; git checkout` here -- you get the same error with that command (dropping the `/bitcoin`) on master afaics?
💬 l0rinc commented on pull request "[IBD] multi-byte block obfuscation":
(https://github.com/bitcoin/bitcoin/pull/31144#issuecomment-3091927892)
Thanks a lot @maflcko, @ryanofsky, @hodlinator and @achow101 for all the reviews and reproducers and suggestions!