💬 Sjors commented on pull request "test: unbreak: exclude windows from wallet_assumeutxo":
(https://github.com/bitcoin/bitcoin/pull/29238#issuecomment-1889777696)
> As a quick fix, what about just removing line 127 which calls `assert_raises_rpc_error`.
That would make the test worse on all platforms. The point of that `assert_raises_rpc_error` is to prevent a regression where we allow the user to restore a wallet too early in the IBD process. That's probably not a rare occurrence: get new computer, start Bitcoin Core, load snapshot, load wallet backup -> oops.
(https://github.com/bitcoin/bitcoin/pull/29238#issuecomment-1889777696)
> As a quick fix, what about just removing line 127 which calls `assert_raises_rpc_error`.
That would make the test worse on all platforms. The point of that `assert_raises_rpc_error` is to prevent a regression where we allow the user to restore a wallet too early in the IBD process. That's probably not a rare occurrence: get new computer, start Bitcoin Core, load snapshot, load wallet backup -> oops.
💬 m3dwards commented on pull request "test: unbreak: exclude windows from wallet_assumeutxo":
(https://github.com/bitcoin/bitcoin/pull/29238#issuecomment-1889779254)
> That failure is an important part of the test since we need to be testing that wallets don't load while background ibd is ongoing.
Understood, was thinking just as a temporary measure until the remove_all call was fixed on windows. Perhaps just disable that line on windows then.
(https://github.com/bitcoin/bitcoin/pull/29238#issuecomment-1889779254)
> That failure is an important part of the test since we need to be testing that wallets don't load while background ibd is ongoing.
Understood, was thinking just as a temporary measure until the remove_all call was fixed on windows. Perhaps just disable that line on windows then.
💬 achow101 commented on pull request "test: add assumeutxo wallet test":
(https://github.com/bitcoin/bitcoin/pull/28838#discussion_r1450807950)
> I think it's more likely that the database isn't fully closed yet even though the `CWallet` should be destructed.
Maybe it's because of `-unsafesqlitesync`, going to try turning that off for the test.
(https://github.com/bitcoin/bitcoin/pull/28838#discussion_r1450807950)
> I think it's more likely that the database isn't fully closed yet even though the `CWallet` should be destructed.
Maybe it's because of `-unsafesqlitesync`, going to try turning that off for the test.
💬 mjdietzx commented on pull request "tests: add functional test for miniscript decaying multisig":
(https://github.com/bitcoin/bitcoin/pull/29156#issuecomment-1889791034)
Thank you @michaelfolkson and @Eunovo for the reviews! I agree with both of your feedback re improving `descriptors.md` docs to accompany this test. I will have a commit addressing your feedback in the coming days.
> I think it is probably overkill to create a new functional test file for this. I'm not sure why it can't go in wallet_miniscript.py
My intention with this test is to provide a "reference example" with the same motivations and flow/architecture as #22067 (and the original issue
...
(https://github.com/bitcoin/bitcoin/pull/29156#issuecomment-1889791034)
Thank you @michaelfolkson and @Eunovo for the reviews! I agree with both of your feedback re improving `descriptors.md` docs to accompany this test. I will have a commit addressing your feedback in the coming days.
> I think it is probably overkill to create a new functional test file for this. I'm not sure why it can't go in wallet_miniscript.py
My intention with this test is to provide a "reference example" with the same motivations and flow/architecture as #22067 (and the original issue
...
💬 Sjors commented on pull request "test: add assumeutxo wallet test":
(https://github.com/bitcoin/bitcoin/pull/28838#discussion_r1450812828)
I wonder why it's called unsafe :-)
(https://github.com/bitcoin/bitcoin/pull/28838#discussion_r1450812828)
I wonder why it's called unsafe :-)
💬 alfonsoromanz commented on pull request "Fix typos":
(https://github.com/bitcoin-core/gui/pull/787#issuecomment-1889805504)
Concept ACK. Maybe it's better to squash both commits into a single one? Not sure what others think
(https://github.com/bitcoin-core/gui/pull/787#issuecomment-1889805504)
Concept ACK. Maybe it's better to squash both commits into a single one? Not sure what others think
🤔 furszy reviewed a pull request: "test: wallet rescan with reorged parent + IsFromMe child in mempool"
(https://github.com/bitcoin/bitcoin/pull/29179#pullrequestreview-1818866732)
Code review ACK df30247705, nits can be disregarded.
(https://github.com/bitcoin/bitcoin/pull/29179#pullrequestreview-1818866732)
Code review ACK df30247705, nits can be disregarded.
💬 furszy commented on pull request "test: wallet rescan with reorged parent + IsFromMe child in mempool":
(https://github.com/bitcoin/bitcoin/pull/29179#discussion_r1450823884)
nit:
leftover; should be "import descriptor on another wallet"
(https://github.com/bitcoin/bitcoin/pull/29179#discussion_r1450823884)
nit:
leftover; should be "import descriptor on another wallet"
💬 furszy commented on pull request "test: wallet rescan with reorged parent + IsFromMe child in mempool":
(https://github.com/bitcoin/bitcoin/pull/29179#discussion_r1450823229)
nit:
leftover
```suggestion
self.log.info("Create a child tx and wait until all wallets are notified")
```
(https://github.com/bitcoin/bitcoin/pull/29179#discussion_r1450823229)
nit:
leftover
```suggestion
self.log.info("Create a child tx and wait until all wallets are notified")
```
💬 Sjors commented on pull request "OP_CHECKTEMPLATEVERIFY, OP_CHECKSIGFROMSTACK(VERIFY), OP_INTERNALKEY validation (LNHANCE)":
(https://github.com/bitcoin/bitcoin/pull/29198#issuecomment-1889817551)
> I'm not aware of a well defined process for making changes to bitcoin consensus (nor do I think there should be one), so I'm not sure how we can be too early in the process.
Here's a good place to start: https://datatracker.ietf.org/doc/html/rfc7282
(Non draft) pull requests to Bitcoin Core are not a good way to find consensus. Opening before such consensus most likely decreases of anyone being motivated to review it.
> CTV had (a year of?) workshops and review with Jeremy Rubin abo
...
(https://github.com/bitcoin/bitcoin/pull/29198#issuecomment-1889817551)
> I'm not aware of a well defined process for making changes to bitcoin consensus (nor do I think there should be one), so I'm not sure how we can be too early in the process.
Here's a good place to start: https://datatracker.ietf.org/doc/html/rfc7282
(Non draft) pull requests to Bitcoin Core are not a good way to find consensus. Opening before such consensus most likely decreases of anyone being motivated to review it.
> CTV had (a year of?) workshops and review with Jeremy Rubin abo
...
💬 theuni commented on pull request "build: depends move macOS C(XX) FLAGS out of C & CXX":
(https://github.com/bitcoin/bitcoin/pull/29233#issuecomment-1889823070)
Hmm. I was curious to see if the compiler forwards `-mmacosx-version-min` to the linker, and it seems it does:
```bash
$ clang -mmacosx-version-min=11.2 test.c -c -o test.o
$ clang -mmacosx-version-min=11.2 test.o -o testing -v
Apple clang version 14.0.3 (clang-1403.0.22.14.1)
Target: arm64-apple-darwin22.5.0
Thread model: posix
InstalledDir: /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin
"/Applications/Xcode.app/Contents/Developer/Toolchain
...
(https://github.com/bitcoin/bitcoin/pull/29233#issuecomment-1889823070)
Hmm. I was curious to see if the compiler forwards `-mmacosx-version-min` to the linker, and it seems it does:
```bash
$ clang -mmacosx-version-min=11.2 test.c -c -o test.o
$ clang -mmacosx-version-min=11.2 test.o -o testing -v
Apple clang version 14.0.3 (clang-1403.0.22.14.1)
Target: arm64-apple-darwin22.5.0
Thread model: posix
InstalledDir: /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin
"/Applications/Xcode.app/Contents/Developer/Toolchain
...
🤔 mzumsande reviewed a pull request: "log mempool loading progress"
(https://github.com/bitcoin/bitcoin/pull/29227#pullrequestreview-1818890399)
tested ACK eb78ea4eebfe150bc1746282bfdad6eb0f764e3c
(https://github.com/bitcoin/bitcoin/pull/29227#pullrequestreview-1818890399)
tested ACK eb78ea4eebfe150bc1746282bfdad6eb0f764e3c
💬 reardencode commented on pull request "OP_CHECKTEMPLATEVERIFY, OP_CHECKSIGFROMSTACK(VERIFY), OP_INTERNALKEY validation (LNHANCE)":
(https://github.com/bitcoin/bitcoin/pull/29198#issuecomment-1889850492)
> > I'm not aware of a well defined process for making changes to bitcoin consensus (nor do I think there should be one), so I'm not sure how we can be too early in the process.
>
> Here's a good place to start: https://datatracker.ietf.org/doc/html/rfc7282
>
> (Non draft) pull requests to Bitcoin Core are not a good way to find consensus. Opening before such consensus most likely decreases of anyone being motivated to review it.
As I mentioned on my [post on delving](https://delvingbit
...
(https://github.com/bitcoin/bitcoin/pull/29198#issuecomment-1889850492)
> > I'm not aware of a well defined process for making changes to bitcoin consensus (nor do I think there should be one), so I'm not sure how we can be too early in the process.
>
> Here's a good place to start: https://datatracker.ietf.org/doc/html/rfc7282
>
> (Non draft) pull requests to Bitcoin Core are not a good way to find consensus. Opening before such consensus most likely decreases of anyone being motivated to review it.
As I mentioned on my [post on delving](https://delvingbit
...
🤔 jonatack reviewed a pull request: "net, cli: use v2transport for manual/addrfetch connections, add to -netinfo"
(https://github.com/bitcoin/bitcoin/pull/29058#pullrequestreview-1818940008)
Post-merge utACK, pending digging a bit deeper into the IRL effects.
(https://github.com/bitcoin/bitcoin/pull/29058#pullrequestreview-1818940008)
Post-merge utACK, pending digging a bit deeper into the IRL effects.
💬 jonatack commented on pull request "net, cli: use v2transport for manual/addrfetch connections, add to -netinfo":
(https://github.com/bitcoin/bitcoin/pull/29058#discussion_r1450871488)
> a single connection / reconnection attempt doesn't waste a meaningful amount of resources of either side
Note that every I2P connection requires building 2 tunnels, one for each direction. These are meant to be long-lived connections, as they take network resources and waiting for tunnels to be built for every peer connection adds delay to connection setup time.
(https://github.com/bitcoin/bitcoin/pull/29058#discussion_r1450871488)
> a single connection / reconnection attempt doesn't waste a meaningful amount of resources of either side
Note that every I2P connection requires building 2 tunnels, one for each direction. These are meant to be long-lived connections, as they take network resources and waiting for tunnels to be built for every peer connection adds delay to connection setup time.
💬 jonatack commented on pull request "net, cli: use v2transport for manual/addrfetch connections, add to -netinfo":
(https://github.com/bitcoin/bitcoin/pull/29058#discussion_r1450864532)
9eed22e nit, code unneeded until/unless there is a `grant`
```diff
@@ -2316,12 +2316,12 @@ void CConnman::ProcessAddrFetch()
strDest = m_addr_fetches.front();
m_addr_fetches.pop_front();
}
- // Attempt v2 connection if we support v2 - we'll reconnect with v1 if our
- // peer doesn't support it or immediately disconnects us for another reason.
- const bool use_v2transport(GetLocalServices() & NODE_P2P_V2);
- CAddress addr;
CSemaphoreGrant grant(*
...
(https://github.com/bitcoin/bitcoin/pull/29058#discussion_r1450864532)
9eed22e nit, code unneeded until/unless there is a `grant`
```diff
@@ -2316,12 +2316,12 @@ void CConnman::ProcessAddrFetch()
strDest = m_addr_fetches.front();
m_addr_fetches.pop_front();
}
- // Attempt v2 connection if we support v2 - we'll reconnect with v1 if our
- // peer doesn't support it or immediately disconnects us for another reason.
- const bool use_v2transport(GetLocalServices() & NODE_P2P_V2);
- CAddress addr;
CSemaphoreGrant grant(*
...
💬 murchandamus commented on pull request "wallet: Add CoinGrinder coin selection algorithm":
(https://github.com/bitcoin/bitcoin/pull/27877#issuecomment-1889878218)
I’ve added another fuzz target that validates the output of CoinGrinder against a bruteforce search for the smallest weight input set.
(https://github.com/bitcoin/bitcoin/pull/27877#issuecomment-1889878218)
I’ve added another fuzz target that validates the output of CoinGrinder against a bruteforce search for the smallest weight input set.
👍 jamesob approved a pull request: "kernel: Remove dependency on CScheduler"
(https://github.com/bitcoin/bitcoin/pull/28960#pullrequestreview-1818940398)
ACK d63b2e88780dc78fd531b053653361a0bf3fcbea ([`jamesob/ackr/28960.1.TheCharlatan.kernel_remove_dependency`](https://github.com/jamesob/bitcoin/tree/ackr/28960.1.TheCharlatan.kernel_remove_dependency))
Reviewed and ran tests locally. Changes are pretty straightforward and common-sense.
Putting event queue execution into the hands of libbitcoinkernel users seems like the right design choice, and it is especially motivating that this change helps novel fuzz testing (#29158).
As an aside,
...
(https://github.com/bitcoin/bitcoin/pull/28960#pullrequestreview-1818940398)
ACK d63b2e88780dc78fd531b053653361a0bf3fcbea ([`jamesob/ackr/28960.1.TheCharlatan.kernel_remove_dependency`](https://github.com/jamesob/bitcoin/tree/ackr/28960.1.TheCharlatan.kernel_remove_dependency))
Reviewed and ran tests locally. Changes are pretty straightforward and common-sense.
Putting event queue execution into the hands of libbitcoinkernel users seems like the right design choice, and it is especially motivating that this change helps novel fuzz testing (#29158).
As an aside,
...
💬 jamesob commented on pull request "kernel: Remove dependency on CScheduler":
(https://github.com/bitcoin/bitcoin/pull/28960#discussion_r1450864735)
https://github.com/bitcoin/bitcoin/pull/28960/commits/d63b2e88780dc78fd531b053653361a0bf3fcbea
I guess this periodic call was originally included to demonstrate how you might use the scheduler from the libbitcoinkernel interface, but now that we've removed the scheduler from the interface there isn't really an equivalent. Clients would be expected to run their own async event managers and periodic schedulers, which makes sense - and I guess is the whole point of this change!
(https://github.com/bitcoin/bitcoin/pull/28960#discussion_r1450864735)
https://github.com/bitcoin/bitcoin/pull/28960/commits/d63b2e88780dc78fd531b053653361a0bf3fcbea
I guess this periodic call was originally included to demonstrate how you might use the scheduler from the libbitcoinkernel interface, but now that we've removed the scheduler from the interface there isn't really an equivalent. Clients would be expected to run their own async event managers and periodic schedulers, which makes sense - and I guess is the whole point of this change!
💬 kristapsk commented on issue "rpc: actually deprecate `rpcuser` & `rpcpass`":
(https://github.com/bitcoin/bitcoin/issues/29240#issuecomment-1889887704)
> If we are going to deprecate these options, now seems like a good time to do so. We could start with updating all exmaples / docs in this repository, to remove any `rpcuser`/`rpcpass` usage.
That should definitely be done first.
One thing with cookie auth, that currently needs hacks, is allowing cookie access for other users. #28167
(https://github.com/bitcoin/bitcoin/issues/29240#issuecomment-1889887704)
> If we are going to deprecate these options, now seems like a good time to do so. We could start with updating all exmaples / docs in this repository, to remove any `rpcuser`/`rpcpass` usage.
That should definitely be done first.
One thing with cookie auth, that currently needs hacks, is allowing cookie access for other users. #28167