💬 davidgumberg commented on issue "guix: update LIEF from 0.13.2 to 0.16.x":
(https://github.com/bitcoin/bitcoin/issues/30520#issuecomment-2856726776)
I've opened #32431 to partially address this issue, it only bumps the version to 0.16.5 without implementing the other suggested improvements here.
(https://github.com/bitcoin/bitcoin/issues/30520#issuecomment-2856726776)
I've opened #32431 to partially address this issue, it only bumps the version to 0.16.5 without implementing the other suggested improvements here.
💬 davidgumberg commented on pull request "random: Use modern Windows randomness functions":
(https://github.com/bitcoin/bitcoin/pull/32400#discussion_r2076607982)
Fixed both issues, thanks.
Actually, it turns out `symbol-check.py` actually complains on master about bcrypt! That's because it gets included by secp, I've opened https://github.com/bitcoin/bitcoin/pull/32431 which includes the same change and also bumps the lief version, I'll rebase (if) whichever of these two PR's get merged first.
(https://github.com/bitcoin/bitcoin/pull/32400#discussion_r2076607982)
Fixed both issues, thanks.
Actually, it turns out `symbol-check.py` actually complains on master about bcrypt! That's because it gets included by secp, I've opened https://github.com/bitcoin/bitcoin/pull/32431 which includes the same change and also bumps the lief version, I'll rebase (if) whichever of these two PR's get merged first.
💬 davidgumberg commented on pull request "random: Use modern Windows randomness functions":
(https://github.com/bitcoin/bitcoin/pull/32400#issuecomment-2856732190)
Rebased to address reviewer feedback.
(https://github.com/bitcoin/bitcoin/pull/32400#issuecomment-2856732190)
Rebased to address reviewer feedback.
🤔 w0xlt reviewed a pull request: "Remove the legacy wallet and BDB dependency"
(https://github.com/bitcoin/bitcoin/pull/28710#pullrequestreview-2820087181)
reACK https://github.com/bitcoin/bitcoin/pull/28710/commits/de054df6dc32cbd8b127c6761d9c65d95025e08f
(https://github.com/bitcoin/bitcoin/pull/28710#pullrequestreview-2820087181)
reACK https://github.com/bitcoin/bitcoin/pull/28710/commits/de054df6dc32cbd8b127c6761d9c65d95025e08f
🤔 dominickbrasileiro reviewed a pull request: "Remove arbitrary limits on OP_Return (datacarrier) outputs"
(https://github.com/bitcoin/bitcoin/pull/32359#pullrequestreview-2820134558)
NACK
I understand the reasoning behind removing the default cap, but it doesn't make sense to remove the `-datacarrier` and `-datacarriersize` config options. Node operators should be able to configure their mempools and filter out data they don't want to store.
If spam is the problem, we should address the spam filter mechanisms instead of simply allowing it to be stored somewhere else.
(https://github.com/bitcoin/bitcoin/pull/32359#pullrequestreview-2820134558)
NACK
I understand the reasoning behind removing the default cap, but it doesn't make sense to remove the `-datacarrier` and `-datacarriersize` config options. Node operators should be able to configure their mempools and filter out data they don't want to store.
If spam is the problem, we should address the spam filter mechanisms instead of simply allowing it to be stored somewhere else.
📝 w0xlt opened a pull request: "wallet, rpc: Change `OutputType` items from `string` into compile-time constants `string_view`"
(https://github.com/bitcoin/bitcoin/pull/32432)
Follow-up to https://github.com/bitcoin/bitcoin/pull/32429, built on top of it.
This PR addresses the https://github.com/bitcoin/bitcoin/pull/32429#discussion_r2076251627 that the RPC documentation does not use `OUTPUT_TYPES`, but rather hardcodes them, as is already the case for the `getnewaddress` command.
So here the output types are changed from `std::string` to `std::string_view` so that the values are known at compile time or during the early stages of program startup, before main() ex
...
(https://github.com/bitcoin/bitcoin/pull/32432)
Follow-up to https://github.com/bitcoin/bitcoin/pull/32429, built on top of it.
This PR addresses the https://github.com/bitcoin/bitcoin/pull/32429#discussion_r2076251627 that the RPC documentation does not use `OUTPUT_TYPES`, but rather hardcodes them, as is already the case for the `getnewaddress` command.
So here the output types are changed from `std::string` to `std::string_view` so that the values are known at compile time or during the early stages of program startup, before main() ex
...
💬 shahsb commented on pull request "(RFC) kernel: Replace leveldb-based BlockTreeDB with flat-file based store":
(https://github.com/bitcoin/bitcoin/pull/32427#issuecomment-2856855525)
Thanks @TheCharlatan for this proposal and making the code changes.!
Please find my review comments, suggestions and some clarifying questions:
1) How will concurrent reads (and potentially writes) be handled with the flat file format?
2) Even if no deletion occurs, file corruption or partial writes can happen. Are you planning mmap or memory buffering?
3) What would be the "Corruption Recovery Strategy?" -- While the write-ahead log is mentioned as future work, providing even a mini
...
(https://github.com/bitcoin/bitcoin/pull/32427#issuecomment-2856855525)
Thanks @TheCharlatan for this proposal and making the code changes.!
Please find my review comments, suggestions and some clarifying questions:
1) How will concurrent reads (and potentially writes) be handled with the flat file format?
2) Even if no deletion occurs, file corruption or partial writes can happen. Are you planning mmap or memory buffering?
3) What would be the "Corruption Recovery Strategy?" -- While the write-ahead log is mentioned as future work, providing even a mini
...
💬 shahsb commented on pull request "(RFC) kernel: Replace leveldb-based BlockTreeDB with flat-file based store":
(https://github.com/bitcoin/bitcoin/pull/32427#issuecomment-2856856921)
Consider including a pluggable interface that allows fallback to LevelDB for testing or backwards compatibility
(https://github.com/bitcoin/bitcoin/pull/32427#issuecomment-2856856921)
Consider including a pluggable interface that allows fallback to LevelDB for testing or backwards compatibility
💬 samurai321 commented on pull request "Remove arbitrary limits on OP_Return (datacarrier) outputs":
(https://github.com/bitcoin/bitcoin/pull/32359#issuecomment-2856862207)
Concept NACK.
how about enabling an official way to store info about a transaction? And close the loopholes.
Just state that OP_RETURN should contain a hash at most and then query that info from an, optional to run, bittorrent DHT. (v2.0 )
(https://github.com/bitcoin/bitcoin/pull/32359#issuecomment-2856862207)
Concept NACK.
how about enabling an official way to store info about a transaction? And close the loopholes.
Just state that OP_RETURN should contain a hash at most and then query that info from an, optional to run, bittorrent DHT. (v2.0 )
💬 shahsb commented on pull request "(RFC) kernel: Replace leveldb-based BlockTreeDB with flat-file based store":
(https://github.com/bitcoin/bitcoin/pull/32427#issuecomment-2856869458)
7. **Write contention and corruption risks:** -- While flat files avoid LevelDB’s process-level locking, concurrent writes require a mechanism (e.g., file locks, flock()) to prevent race conditions.
8. **Portability and Cross platform related edge cases: ** -- Ensure file-locking mechanisms (e.g., fcntl on Unix, LockFileEx on Windows) are robust.
(https://github.com/bitcoin/bitcoin/pull/32427#issuecomment-2856869458)
7. **Write contention and corruption risks:** -- While flat files avoid LevelDB’s process-level locking, concurrent writes require a mechanism (e.g., file locks, flock()) to prevent race conditions.
8. **Portability and Cross platform related edge cases: ** -- Ensure file-locking mechanisms (e.g., fcntl on Unix, LockFileEx on Windows) are robust.
💬 shahsb commented on pull request "(RFC) kernel: Replace leveldb-based BlockTreeDB with flat-file based store":
(https://github.com/bitcoin/bitcoin/pull/32427#issuecomment-2856870786)
9. How about Handling Large Files? -- Test edge cases like file sizes approaching OS limits (e.g., 2+ GB on 32-bit systems). What would happen in such cases?
(https://github.com/bitcoin/bitcoin/pull/32427#issuecomment-2856870786)
9. How about Handling Large Files? -- Test edge cases like file sizes approaching OS limits (e.g., 2+ GB on 32-bit systems). What would happen in such cases?
💬 shahsb commented on pull request "(RFC) kernel: Replace leveldb-based BlockTreeDB with flat-file based store":
(https://github.com/bitcoin/bitcoin/pull/32427#issuecomment-2856877928)
10. On similar lines to point #5 -- **Corruption Detection** mechanism could also be implemented to detect corruptions very early in the cycle.
(https://github.com/bitcoin/bitcoin/pull/32427#issuecomment-2856877928)
10. On similar lines to point #5 -- **Corruption Detection** mechanism could also be implemented to detect corruptions very early in the cycle.
🤔 shahsb reviewed a pull request: "(RFC) kernel: Replace leveldb-based BlockTreeDB with flat-file based store"
(https://github.com/bitcoin/bitcoin/pull/32427#pullrequestreview-2820208053)
The flat-file approach is a reasonable trade-off given the simplicity of the block tree storage requirements.
However, there are major significant challenges and risks involved with this approach as highlighted in the above 10 comments. (Concurrency, Corruption/Integrity, Performance, Migration, Corruption detection, Portability, Backward compatibility etc.)
(https://github.com/bitcoin/bitcoin/pull/32427#pullrequestreview-2820208053)
The flat-file approach is a reasonable trade-off given the simplicity of the block tree storage requirements.
However, there are major significant challenges and risks involved with this approach as highlighted in the above 10 comments. (Concurrency, Corruption/Integrity, Performance, Migration, Corruption detection, Portability, Backward compatibility etc.)
💬 w0xlt commented on pull request "docs: Improve `keypoolrefill` RPC docs":
(https://github.com/bitcoin/bitcoin/pull/32429#discussion_r2076693232)
`OUTPUT_TYPES` handled in https://github.com/bitcoin/bitcoin/pull/32432
(https://github.com/bitcoin/bitcoin/pull/32429#discussion_r2076693232)
`OUTPUT_TYPES` handled in https://github.com/bitcoin/bitcoin/pull/32432
💬 ajtowns commented on pull request "policy: uncap datacarrier by default":
(https://github.com/bitcoin/bitcoin/pull/32406#discussion_r2076791280)
"multiple datacarrier outputs within a transaction"
(https://github.com/bitcoin/bitcoin/pull/32406#discussion_r2076791280)
"multiple datacarrier outputs within a transaction"
💬 Zodomo commented on pull request "Remove arbitrary limits on OP_Return (datacarrier) outputs":
(https://github.com/bitcoin/bitcoin/pull/32359#issuecomment-2857093636)
Concept ACK
The message Satoshi Nakamoto inscribed into the Genesis Block is 69 bytes, yet many advocate for reducing it to ~40. This change would have prevented Bitcoin's emergent criticism of the existing fiat system from being included. Satoshi stored "arbitrary data" in the very first block. To say that such usage is "spam" is to ignore Bitcoin's origin.
The world's most neutral and immutable ledger should be unopinionated towards what the free market would like, and pays for it to include
...
(https://github.com/bitcoin/bitcoin/pull/32359#issuecomment-2857093636)
Concept ACK
The message Satoshi Nakamoto inscribed into the Genesis Block is 69 bytes, yet many advocate for reducing it to ~40. This change would have prevented Bitcoin's emergent criticism of the existing fiat system from being included. Satoshi stored "arbitrary data" in the very first block. To say that such usage is "spam" is to ignore Bitcoin's origin.
The world's most neutral and immutable ledger should be unopinionated towards what the free market would like, and pays for it to include
...
💬 laanwj commented on pull request "deps: Bump lief to 0.16.5":
(https://github.com/bitcoin/bitcoin/pull/32431#issuecomment-2857144478)
Concept ACK.
> Fixes an existing bug, where bcrypt.dll was not one of the expected symbols in contrib/devtools/symbol-check.py, even though this symbol gets included by way of secp256k1:
From what i understand, that's only used for the example, not for the library itself. The bcrypt-using code (`fill_random` in `examples/examples_util.h`) is never included in the library.
It's unlikely that the LIEF version changes anything there.
(https://github.com/bitcoin/bitcoin/pull/32431#issuecomment-2857144478)
Concept ACK.
> Fixes an existing bug, where bcrypt.dll was not one of the expected symbols in contrib/devtools/symbol-check.py, even though this symbol gets included by way of secp256k1:
From what i understand, that's only used for the example, not for the library itself. The bcrypt-using code (`fill_random` in `examples/examples_util.h`) is never included in the library.
It's unlikely that the LIEF version changes anything there.
👍 laanwj approved a pull request: "random: Use modern Windows randomness functions"
(https://github.com/bitcoin/bitcoin/pull/32400#pullrequestreview-2820459818)
Code review ACK 345944bd6cf8be0c8b85b6d3ccb593ce26c598aa
Code changes look correct. i *haven't* tested this on an actual windows machine.
(https://github.com/bitcoin/bitcoin/pull/32400#pullrequestreview-2820459818)
Code review ACK 345944bd6cf8be0c8b85b6d3ccb593ce26c598aa
Code changes look correct. i *haven't* tested this on an actual windows machine.
🤔 hebasto reviewed a pull request: "deps: Bump lief to 0.16.5"
(https://github.com/bitcoin/bitcoin/pull/32431#pullrequestreview-2820462645)
Also the LIEF version must be bumped here: https://github.com/bitcoin/bitcoin/blob/59d3e4ed34eb55cb40928d524cb0bd5e183ed85a/contrib/guix/manifest.scm#L154-L161
(https://github.com/bitcoin/bitcoin/pull/32431#pullrequestreview-2820462645)
Also the LIEF version must be bumped here: https://github.com/bitcoin/bitcoin/blob/59d3e4ed34eb55cb40928d524cb0bd5e183ed85a/contrib/guix/manifest.scm#L154-L161
💬 AlexSQY commented on pull request "CAT in Tapscript (BIP-347)":
(https://github.com/bitcoin/bitcoin/pull/29247#issuecomment-2857183685)
What about SCRIPT_VERIFY OP_CAT = False and SCRIPT_VERIFY DISCOURAGE_OP_CAT = False?
This combination is not described in the "soft fork" table. Is it part of the tests?
(https://github.com/bitcoin/bitcoin/pull/29247#issuecomment-2857183685)
What about SCRIPT_VERIFY OP_CAT = False and SCRIPT_VERIFY DISCOURAGE_OP_CAT = False?
This combination is not described in the "soft fork" table. Is it part of the tests?