💬 Sjors commented on pull request "doc: add LLVM instruction for macOS < 13":
(https://github.com/bitcoin/bitcoin/pull/29934#discussion_r1575067094)
That's not that long ago though, and we have `OSX_MIN_VERSION=11.0` for now.
(https://github.com/bitcoin/bitcoin/pull/29934#discussion_r1575067094)
That's not that long ago though, and we have `OSX_MIN_VERSION=11.0` for now.
💬 laanwj commented on issue "RFC: Formal description of the RPC API":
(https://github.com/bitcoin/bitcoin/issues/29912#issuecomment-2070215202)
> I vaguely recall an issue or discussion around machine-readable API specs in the past, but I cannot find it. I believe @laanwj commented on it.
Much of it is already there, just not exposed. The `RPCHelpMan`/`RPCResult` structure for `help` handling was intended to be a start of formalizing the API, and to have some form of introspection. E.g. types are already checked against the spec while testing. It could be extended to include other data that's needed.
The same data that's used for
...
(https://github.com/bitcoin/bitcoin/issues/29912#issuecomment-2070215202)
> I vaguely recall an issue or discussion around machine-readable API specs in the past, but I cannot find it. I believe @laanwj commented on it.
Much of it is already there, just not exposed. The `RPCHelpMan`/`RPCResult` structure for `help` handling was intended to be a start of formalizing the API, and to have some form of introspection. E.g. types are already checked against the spec while testing. It could be extended to include other data that's needed.
The same data that's used for
...
🤔 fjahr reviewed a pull request: "test: Validate UTXO snapshot with coin height > base height & amount > MAX_MONEY supply"
(https://github.com/bitcoin/bitcoin/pull/29617#pullrequestreview-2015164071)
Code review ACK 6428b363e68fee51a455ebd4f9bf7ed6d813caa8
Ignore the nit unless there is other feedback to address or you need to rebase.
(https://github.com/bitcoin/bitcoin/pull/29617#pullrequestreview-2015164071)
Code review ACK 6428b363e68fee51a455ebd4f9bf7ed6d813caa8
Ignore the nit unless there is other feedback to address or you need to rebase.
💬 fjahr commented on pull request "test: Validate UTXO snapshot with coin height > base height & amount > MAX_MONEY supply":
(https://github.com/bitcoin/bitcoin/pull/29617#discussion_r1574985012)
nit: `exceeds`
(https://github.com/bitcoin/bitcoin/pull/29617#discussion_r1574985012)
nit: `exceeds`
💬 levantah commented on issue "Release schedule for 27.0":
(https://github.com/bitcoin/bitcoin/issues/29028#issuecomment-2070239920)
This may be also off-topic. Feel free to delete even without reading.
Just to explain where I am coming from, there was a Chaincode Labs Bitcoin seminar done partly by some Core developers and for each run of a test-case it would download Bitcoin binary to Github infrastructure by calling this: `wget -q https://bitcoincore.org/bin/bitcoin-core-26.0/bitcoin-26.0-x86_64-linux-gnu.tar.gz`. It was rather slow. Now imagine hundreds of students. A Bottleneck I would say.
I have no idea how someo
...
(https://github.com/bitcoin/bitcoin/issues/29028#issuecomment-2070239920)
This may be also off-topic. Feel free to delete even without reading.
Just to explain where I am coming from, there was a Chaincode Labs Bitcoin seminar done partly by some Core developers and for each run of a test-case it would download Bitcoin binary to Github infrastructure by calling this: `wget -q https://bitcoincore.org/bin/bitcoin-core-26.0/bitcoin-26.0-x86_64-linux-gnu.tar.gz`. It was rather slow. Now imagine hundreds of students. A Bottleneck I would say.
I have no idea how someo
...
🤔 instagibbs reviewed a pull request: "p2p: opportunistically accept 1-parent-1-child packages"
(https://github.com/bitcoin/bitcoin/pull/28970#pullrequestreview-2015296846)
reviewed through 7d220c6a5c0e0c5e8cfe79ebd2eae6e845d1d983
tested and confirmed fuzz coverage is hitting meaningful `GetChildrenFrom*` results
continuing longer range testing
(https://github.com/bitcoin/bitcoin/pull/28970#pullrequestreview-2015296846)
reviewed through 7d220c6a5c0e0c5e8cfe79ebd2eae6e845d1d983
tested and confirmed fuzz coverage is hitting meaningful `GetChildrenFrom*` results
continuing longer range testing
💬 instagibbs commented on pull request "p2p: opportunistically accept 1-parent-1-child packages":
(https://github.com/bitcoin/bitcoin/pull/28970#discussion_r1575059642)
nit
```Suggestion
BOOST_CHECK(orphanage.GetChildrenFromSamePeer(parent1, node2).empty());
BOOST_CHECK(orphanage.GetChildrenFromSamePeer(parent2, node2).empty());
```
(https://github.com/bitcoin/bitcoin/pull/28970#discussion_r1575059642)
nit
```Suggestion
BOOST_CHECK(orphanage.GetChildrenFromSamePeer(parent1, node2).empty());
BOOST_CHECK(orphanage.GetChildrenFromSamePeer(parent2, node2).empty());
```
💬 instagibbs commented on pull request "p2p: opportunistically accept 1-parent-1-child packages":
(https://github.com/bitcoin/bitcoin/pull/28970#discussion_r1575061630)
nit
```Suggestion
// There shouldn't be any children of this tx in orphanage
```
(https://github.com/bitcoin/bitcoin/pull/28970#discussion_r1575061630)
nit
```Suggestion
// There shouldn't be any children of this tx in orphanage
```
💬 Sjors commented on pull request "[DO NOT MERGE] testnet4 including PoW difficulty adjustment fix":
(https://github.com/bitcoin/bitcoin/pull/29775#issuecomment-2070283429)
> it breaks the use case of someone testing being able to mine a block on-demand without actual mining hardware
I doubt many people do that. You can still set `nProofOfWorkLimit` to a lower value. We could add a code comment for that (or a setting). That way you can mine locally as fast as you want, without causing mayhem for others.
(https://github.com/bitcoin/bitcoin/pull/29775#issuecomment-2070283429)
> it breaks the use case of someone testing being able to mine a block on-demand without actual mining hardware
I doubt many people do that. You can still set `nProofOfWorkLimit` to a lower value. We could add a code comment for that (or a setting). That way you can mine locally as fast as you want, without causing mayhem for others.
💬 hebasto commented on pull request "doc: suggest only necessary Qt packages for installation on FreeBSD":
(https://github.com/bitcoin/bitcoin/pull/29932#discussion_r1575115643)
1. Why `qt5-qmake`?
2. The build system (`build-aux/m4/bitcoin_qt.m4`) checks for the `qt5-network` and `qt-dbus` packages, which both are dependencies of the `qt5-gui`. However, only `qt5-network` is listed explicitly. For consistency sake, I suggest to list either both packages or none.
(https://github.com/bitcoin/bitcoin/pull/29932#discussion_r1575115643)
1. Why `qt5-qmake`?
2. The build system (`build-aux/m4/bitcoin_qt.m4`) checks for the `qt5-network` and `qt-dbus` packages, which both are dependencies of the `qt5-gui`. However, only `qt5-network` is listed explicitly. For consistency sake, I suggest to list either both packages or none.
💬 mzumsande commented on pull request "net: attempts to connect to all resolved addresses when connecting to a node":
(https://github.com/bitcoin/bitcoin/pull/28834#issuecomment-2070355480)
re-ACK fd81a37239541d0d508402cd4eeb28f38128c1f2 (just looked at diff, only small logging change)
(https://github.com/bitcoin/bitcoin/pull/28834#issuecomment-2070355480)
re-ACK fd81a37239541d0d508402cd4eeb28f38128c1f2 (just looked at diff, only small logging change)
💬 mjdietzx commented on pull request "tests: add functional test for miniscript decaying multisig":
(https://github.com/bitcoin/bitcoin/pull/29156#issuecomment-2070481115)
Thank you for the thorough review @achow101! I'll update this PR addressing all of your comments and ping you when I've pushed (I'll need to find a chunk of time to do this so it may be a week or two)
(https://github.com/bitcoin/bitcoin/pull/29156#issuecomment-2070481115)
Thank you for the thorough review @achow101! I'll update this PR addressing all of your comments and ping you when I've pushed (I'll need to find a chunk of time to do this so it may be a week or two)
💬 sr-gi commented on pull request "p2p: gives seednode priority over dnsseed if both are provided":
(https://github.com/bitcoin/bitcoin/pull/28016#issuecomment-2070486303)
Rebased to address merge conflicts
(https://github.com/bitcoin/bitcoin/pull/28016#issuecomment-2070486303)
Rebased to address merge conflicts
💬 sr-gi commented on pull request "net: Favor peers from addrman over fetching seednodes":
(https://github.com/bitcoin/bitcoin/pull/29605#issuecomment-2070491742)
Rebased to fix merge conflicts
(https://github.com/bitcoin/bitcoin/pull/29605#issuecomment-2070491742)
Rebased to fix merge conflicts
🤔 mzumsande reviewed a pull request: "bugfix: update chainman best_header after block reconsideration"
(https://github.com/bitcoin/bitcoin/pull/29913#pullrequestreview-2015537357)
IIn my opinion setting it to `ActiveTip()` is not correct because `m_best_header` was not necessarily equal to `ActiveTip()` in the first place (before `invalidateblock`) :
During IBD they differ a lot (and `invalidateblock` / `resonsiderblock` is allowed then, too!). So after `resonsiderblock` it would still be set to the wrong value. I could verify that on signet via `getblockchaininfo` shortly after downloading the headers.
I think that the correct approach would be to iterate through h
...
(https://github.com/bitcoin/bitcoin/pull/29913#pullrequestreview-2015537357)
IIn my opinion setting it to `ActiveTip()` is not correct because `m_best_header` was not necessarily equal to `ActiveTip()` in the first place (before `invalidateblock`) :
During IBD they differ a lot (and `invalidateblock` / `resonsiderblock` is allowed then, too!). So after `resonsiderblock` it would still be set to the wrong value. I could verify that on signet via `getblockchaininfo` shortly after downloading the headers.
I think that the correct approach would be to iterate through h
...
⚠️ laanwj opened an issue: "guix: SOURCE_DATE_EPOCH is already set in some environments"
(https://github.com/bitcoin/bitcoin/issues/29935)
The environment variable `SOURCE_DATE_EPOCH` allows overriding the date that will be used inside the archives for guix-built binaries. This is an intentional feature, as documented in `contrib/guix/README.md`:
> * _**SOURCE_DATE_EPOCH**_
>
> Override the reference UNIX timestamp used for bit-for-bit reproducibility,
> the variable name conforms to [standard][r12e/source-date-epoch].
>
> _(defaults to the output of `$(git log --format=%at -1)`)_
However, some environments, as a
...
(https://github.com/bitcoin/bitcoin/issues/29935)
The environment variable `SOURCE_DATE_EPOCH` allows overriding the date that will be used inside the archives for guix-built binaries. This is an intentional feature, as documented in `contrib/guix/README.md`:
> * _**SOURCE_DATE_EPOCH**_
>
> Override the reference UNIX timestamp used for bit-for-bit reproducibility,
> the variable name conforms to [standard][r12e/source-date-epoch].
>
> _(defaults to the output of `$(git log --format=%at -1)`)_
However, some environments, as a
...
💬 hebasto commented on issue "guix: SOURCE_DATE_EPOCH is already set in some environments":
(https://github.com/bitcoin/bitcoin/issues/29935#issuecomment-2070764504)
> * Rename our `SOURCE_DATE_EPOCH` to something non-standard that doesn't conflict with Nix.
As we run Guix shell in a container, it seems reasonable to rename `SOURCE_DATE_EPOCH` in the `guix-build` script, and pass it to the container using its original name:
```
--env SOURCE_DATE_EPOCH="${BITCOIN_SOURCE_DATE_EPOCH:?unable to determine value}"
```
(https://github.com/bitcoin/bitcoin/issues/29935#issuecomment-2070764504)
> * Rename our `SOURCE_DATE_EPOCH` to something non-standard that doesn't conflict with Nix.
As we run Guix shell in a container, it seems reasonable to rename `SOURCE_DATE_EPOCH` in the `guix-build` script, and pass it to the container using its original name:
```
--env SOURCE_DATE_EPOCH="${BITCOIN_SOURCE_DATE_EPOCH:?unable to determine value}"
```
📝 brunoerg opened a pull request: "fuzz: wallet: add target for `CreateTransaction`"
(https://github.com/bitcoin/bitcoin/pull/29936)
This PR adds a fuzz target for the `CreateTransaction` function. It is a regression target for https://github.com/bitcoin/bitcoin/pull/27271 and can be testing by applying:
```diff
@@ -1110,7 +1110,7 @@ static util::Result<CreatedTransactionResult> CreateTransactionInternal(
// This can only happen if feerate is 0, and requested destinations are value of 0 (e.g. OP_RETURN)
// and no pre-selected inputs. This will result in 0-input transaction, which is consensus-invalid anyways
...
(https://github.com/bitcoin/bitcoin/pull/29936)
This PR adds a fuzz target for the `CreateTransaction` function. It is a regression target for https://github.com/bitcoin/bitcoin/pull/27271 and can be testing by applying:
```diff
@@ -1110,7 +1110,7 @@ static util::Result<CreatedTransactionResult> CreateTransactionInternal(
// This can only happen if feerate is 0, and requested destinations are value of 0 (e.g. OP_RETURN)
// and no pre-selected inputs. This will result in 0-input transaction, which is consensus-invalid anyways
...
💬 brunoerg commented on issue "Wallet fuzzing tracking issue":
(https://github.com/bitcoin/bitcoin/issues/29901#issuecomment-2070810257)
Just added https://github.com/bitcoin/bitcoin/pull/29936 to the list. It's a regression to #27271.
(https://github.com/bitcoin/bitcoin/issues/29901#issuecomment-2070810257)
Just added https://github.com/bitcoin/bitcoin/pull/29936 to the list. It's a regression to #27271.
💬 achow101 commented on pull request "wallet: Implement independent BDB parser":
(https://github.com/bitcoin/bitcoin/pull/26606#discussion_r1575302127)
Good catch, fixed
(https://github.com/bitcoin/bitcoin/pull/26606#discussion_r1575302127)
Good catch, fixed