💬 sipa commented on pull request "Miniscript: always treat unsatisfiable scripts as insane":
(https://github.com/bitcoin/bitcoin/pull/27997#issuecomment-1614821612)
The requirement for a signature actually stems from the same question, though it's not as exact.

If a script can be satisfied without signatures, it means there may be no signatures at all in the whole transaction or input, and thus nothing that actually commits to the nLockTime or nSequence values respectively, and so an attacker could just modify those values without invalidating anything.

💬 sr-gi commented on pull request "p2p: gives seednode priority over dnsseed if both are provided":
(https://github.com/bitcoin/bitcoin/pull/28016#discussion_r1248003587)
Should be fixed in ac7ce24

🚀 fanquake merged a pull request: "contrib: add macOS test for fixup_chains usage"
(https://github.com/bitcoin/bitcoin/pull/27999)

💬 achow101 commented on pull request "wallet: Implement independent BDB parser":
(https://github.com/bitcoin/bitcoin/pull/26606#discussion_r1248008111)
The record data should just be arbitrary byte strings so they don't have an endianness.

💬 achow101 commented on pull request "wallet: Implement independent BDB parser":
(https://github.com/bitcoin/bitcoin/pull/26606#discussion_r1248013942)
Done

💬 achow101 commented on pull request "wallet: Implement independent BDB parser":
(https://github.com/bitcoin/bitcoin/pull/26606#discussion_r1248014091)
Done as suggested

💬 achow101 commented on pull request "wallet: Implement independent BDB parser":
(https://github.com/bitcoin/bitcoin/pull/26606#discussion_r1248014465)
Removed. I think originally I though I could put the overflow page data into this field, but I didn't end up doing it that way.

💬 Sjors commented on pull request "guix: Clean up manifest":
(https://github.com/bitcoin/bitcoin/pull/27811#issuecomment-1614851497)
I get the same hashes.

💬 pablomartin4btc commented on pull request "httpserver, rest: improving URI validation":
(https://github.com/bitcoin/bitcoin/pull/27253#discussion_r1248024010)
Not anymore, there was an `Assume()` somewhere in `HTTPRequest::GetQueryParameter` checking for `m_uri_parsed` at some point. Thanks for checking, I'll removed it.

💬 achow101 commented on pull request "wallet: Implement independent BDB parser":
(https://github.com/bitcoin/bitcoin/pull/26606#discussion_r1248057852)
IIRC I did this to avoid issues with old wallets that the wallet loading logic would do automatic upgrading of which expects writing to not fail.

👋 fanquake's pull request is ready for review: "[25.x] Parallel compact block downloads"
(https://github.com/bitcoin/bitcoin/pull/27752)

💬 fanquake commented on pull request "build: LLVM 16 & LLD based macOS toolchain":
(https://github.com/bitcoin/bitcoin/pull/21778#issuecomment-1614912597)
Rebased past #27999. Now that we require `fixup_chains`, we also require at least LLVM 16+ (which doesn't yet exist in Guix). Hopefully we'll have the infra to construct it ourselves shortly: https://debbugs.gnu.org/cgi/bugreport.cgi?bug=64100 (which will mean a more recent time-machine bump). https://reviews.llvm.org/D151926 / https://reviews.llvm.org/D151944 has also landed in LLVM (17.x), so we can backport if needed.

In the meantime, I've rebased this, and updated depends to use LLVM 16.0
...

💬 fanquake commented on pull request "guix: Use LTO to build releases":
(https://github.com/bitcoin/bitcoin/pull/25391#issuecomment-1614914758)
Rebased this on top of #27897. Dropped the commit to backport a patch to GCC 10.

💬 fanquake commented on pull request "build: LLVM 16 & LLD based macOS toolchain":
(https://github.com/bitcoin/bitcoin/pull/21778#issuecomment-1614924588)
Also added a commit to drop explicit `-bind_at_load` usage from configure for now. Cleaned up the PR description and instructions. Note that Guix build are basically expected to fail at this point. However if anyone would like to test, cross-compilation can be done using depends.

💬 achow101 commented on pull request "addrman: select addresses by network follow-up":
(https://github.com/bitcoin/bitcoin/pull/27745#issuecomment-1614950818)
ACK cd8ef5b3e66b3f766c9c883259b5feb44540d7df

Checked that the changes here resolve the remaining review comments on #27214

💬 mzumsande commented on pull request "index: make startup more efficient":
(https://github.com/bitcoin/bitcoin/pull/27607#discussion_r1248094492)
I think I also like the `src/node` option most, but it's ok for me to keep in init for now and move that later.

💬 mzumsande commented on pull request "index: make startup more efficient":
(https://github.com/bitcoin/bitcoin/pull/27607#discussion_r1247996232)
Although `GetFirstStoredBlock()` is currently used only for rpc and indexes, this added assert makes me a bit uneasy. While we can check that `GetFirstStoredBlock()` is only called with the tip, how sure can we that the tip has `BLOCK_HAVE_DATA`?
I could think of several edge cases where we don't have data for the tip, and managed to trigger the assert in some of these situations:
1. (tested): With AssumeUTXO, right after loading a chainstate from disk / before getting any additional blocks fr
...

🚀 achow101 merged a pull request: "addrman: select addresses by network follow-up"
(https://github.com/bitcoin/bitcoin/pull/27745)

💬 pablomartin4btc commented on pull request "httpserver, rest: improving URI validation":
(https://github.com/bitcoin/bitcoin/pull/27253#discussion_r1248139828)
I understand your worries on coupling the validation within the constructor (separation of/ different concerns, error handling, flexibility, etc) but since those validations/ "early checks" also would impact into the state of the object itself it would make sense to me to put them together, consistently and cleaner. Happy to make the change if @vasild also agree with it.

💬 sipa commented on pull request "Add support for RFC8439 variant of ChaCha20":
(https://github.com/bitcoin/bitcoin/pull/27985#issuecomment-1615002228)
I've encapsulated the 96-bit nonce into a `using Nonce96 = std::pair<uint32_t, uint64_t>`, which makes the interfaces a bit cleaner.