🤔 achow101 reviewed a pull request: "wallet: optimize migration process, batch db transactions"
(https://github.com/bitcoin/bitcoin/pull/28574#pullrequestreview-2152124552)
Concept ACK
I'm not a huge fan of all the boilerplate and wrappers that this ends up adding, but I also don't see a better way to do this.
In `ApplyMigrationData`, it looks like we're creating batchs for the watchonly and solvable wallets multiple times. Those could be consolidated in this PR as well.
(https://github.com/bitcoin/bitcoin/pull/28574#pullrequestreview-2152124552)
Concept ACK
I'm not a huge fan of all the boilerplate and wrappers that this ends up adding, but I also don't see a better way to do this.
In `ApplyMigrationData`, it looks like we're creating batchs for the watchonly and solvable wallets multiple times. Those could be consolidated in this PR as well.
💬 achow101 commented on pull request "wallet: optimize migration process, batch db transactions":
(https://github.com/bitcoin/bitcoin/pull/28574#discussion_r1661464266)
In 4e72821e1961b58559f49a4746bc8d15d116f6b7 "wallet: ApplyMigrationData, group all db txs into a single atomic operation"
This could be its own commit.
(https://github.com/bitcoin/bitcoin/pull/28574#discussion_r1661464266)
In 4e72821e1961b58559f49a4746bc8d15d116f6b7 "wallet: ApplyMigrationData, group all db txs into a single atomic operation"
This could be its own commit.
💬 achow101 commented on pull request "rpc: Avoid getchaintxstats invalid results":
(https://github.com/bitcoin/bitcoin/pull/29720#discussion_r1661483289)
In faa9a1fcd05341c1cbe9b2e2257c1c0248b2a4d4 "rpc: Avoid getchaintxstats invalid results"
typo: "start end end". I think that's supposed to be "start and end"
```suggestion
"Only returned if \"window_block_count\" is > 0 and if txcount exists for the start and end of the window."},
```
(https://github.com/bitcoin/bitcoin/pull/29720#discussion_r1661483289)
In faa9a1fcd05341c1cbe9b2e2257c1c0248b2a4d4 "rpc: Avoid getchaintxstats invalid results"
typo: "start end end". I think that's supposed to be "start and end"
```suggestion
"Only returned if \"window_block_count\" is > 0 and if txcount exists for the start and end of the window."},
```
✅ ceffikhan closed a pull request: "test: fix debug log assertion in p2p_i2p_ports test"
(https://github.com/bitcoin/bitcoin/pull/30345)
(https://github.com/bitcoin/bitcoin/pull/30345)
💬 ceffikhan commented on pull request "test: fix debug log assertion in p2p_i2p_ports test":
(https://github.com/bitcoin/bitcoin/pull/30345#issuecomment-2200940397)
closing PR for now
(https://github.com/bitcoin/bitcoin/pull/30345#issuecomment-2200940397)
closing PR for now
💬 paplorinc commented on pull request "optimization: Speed up Base58 encoding/decoding by 400%/200% via preliminary byte packing":
(https://github.com/bitcoin/bitcoin/pull/29473#issuecomment-2200977622)
> Ok, the reason for https://github.com/bitcoin/bitcoin/pull/7656#issue-139438690 was an improvement in listunspent. Seems fine, if this is still the case. But this will need to be checked first.
I've created 10k blocks in regtest and measured the performance of `listunspent` before and after:
```bash
killall bitcoind 2>/dev/null || true
./src/bitcoind -regtest -daemon
./src/bitcoin-cli -regtest createwallet "test_wallet"
./src/bitcoin-cli -regtest generatetoaddress 10000 $(./src/bitco
...
(https://github.com/bitcoin/bitcoin/pull/29473#issuecomment-2200977622)
> Ok, the reason for https://github.com/bitcoin/bitcoin/pull/7656#issue-139438690 was an improvement in listunspent. Seems fine, if this is still the case. But this will need to be checked first.
I've created 10k blocks in regtest and measured the performance of `listunspent` before and after:
```bash
killall bitcoind 2>/dev/null || true
./src/bitcoind -regtest -daemon
./src/bitcoin-cli -regtest createwallet "test_wallet"
./src/bitcoin-cli -regtest generatetoaddress 10000 $(./src/bitco
...
💬 pablomartin4btc commented on pull request "cli: improve error message on multiwallet and add validation to cli-side commands":
(https://github.com/bitcoin/bitcoin/pull/26990#issuecomment-2200986730)
> Also, "protocol.h" in the first commit message body seems wrong or out-of-date here: `bringing the error message in line with the definition established in protocol.h ("error when there are multiple wallets loaded")`. I'm not sure, but did you mean "src/wallet/rpc/util.cpp" instead?
The reference to "`src/rpc/protocol.h`" was intended to clarify the context of the error message alignment, specifically in the section of the code where the RPC error code "not wallet specified" is defined (`en
...
(https://github.com/bitcoin/bitcoin/pull/26990#issuecomment-2200986730)
> Also, "protocol.h" in the first commit message body seems wrong or out-of-date here: `bringing the error message in line with the definition established in protocol.h ("error when there are multiple wallets loaded")`. I'm not sure, but did you mean "src/wallet/rpc/util.cpp" instead?
The reference to "`src/rpc/protocol.h`" was intended to clarify the context of the error message alignment, specifically in the section of the code where the RPC error code "not wallet specified" is defined (`en
...
💬 pablomartin4btc commented on pull request "cli: improve error message on multiwallet and add validation to cli-side commands":
(https://github.com/bitcoin/bitcoin/pull/26990#discussion_r1661528309)
Ok, thanks for checking, I'll work on it and will incorporate full error message in the functional test.
(https://github.com/bitcoin/bitcoin/pull/26990#discussion_r1661528309)
Ok, thanks for checking, I'll work on it and will incorporate full error message in the functional test.
💬 brunoerg commented on issue "Intermittent issue in p2p_i2p_ports.py AssertionError: [node 0] Expected messages "['Error connecting to [...].b32.i2p:0: Cannot connect to 127.0.0.1:60000']" does not partially match log:":
(https://github.com/bitcoin/bitcoin/issues/30030#issuecomment-2201034319)
See https://github.com/bitcoin/bitcoin/pull/30345#issuecomment-2200659833
It seems there is something listening at port 60000 in parallel.
(https://github.com/bitcoin/bitcoin/issues/30030#issuecomment-2201034319)
See https://github.com/bitcoin/bitcoin/pull/30345#issuecomment-2200659833
It seems there is something listening at port 60000 in parallel.
💬 fjahr commented on pull request "#27307 follow-up: update mempool conflict tests + docs":
(https://github.com/bitcoin/bitcoin/pull/30365#issuecomment-2201099336)
ACK 7d55796c530f891493302059c6200d4a606c1ca9
The CI failures are related to #30368, not this change.
(https://github.com/bitcoin/bitcoin/pull/30365#issuecomment-2201099336)
ACK 7d55796c530f891493302059c6200d4a606c1ca9
The CI failures are related to #30368, not this change.
💬 cbergqvist commented on pull request "wallet: Migrate legacy wallets to descriptor wallets without requiring BDB":
(https://github.com/bitcoin/bitcoin/pull/26596#discussion_r1661566826)
(Now grepping "AddKeyPubKeyInner(const CKey& key, const CPubKey& pubkey)" gives 1 result.
grepping "AddKeyPubKeyInner(const CKey& key, const CPubKey &pubkey)" gives 3 results.
It would be nice to align style within the same class between .CPP/.H).
(https://github.com/bitcoin/bitcoin/pull/26596#discussion_r1661566826)
(Now grepping "AddKeyPubKeyInner(const CKey& key, const CPubKey& pubkey)" gives 1 result.
grepping "AddKeyPubKeyInner(const CKey& key, const CPubKey &pubkey)" gives 3 results.
It would be nice to align style within the same class between .CPP/.H).
💬 achow101 commented on pull request "wallet: Migrate legacy wallets to descriptor wallets without requiring BDB":
(https://github.com/bitcoin/bitcoin/pull/26596#discussion_r1661578955)
Not going to change existing code, especially code that will be deleted, to match style
(https://github.com/bitcoin/bitcoin/pull/26596#discussion_r1661578955)
Not going to change existing code, especially code that will be deleted, to match style
💬 cbergqvist commented on pull request "wallet: Migrate legacy wallets to descriptor wallets without requiring BDB":
(https://github.com/bitcoin/bitcoin/pull/26596#discussion_r1661582339)
(Agreed, but then I would skip doing the suggested change in the .CPP file).
(https://github.com/bitcoin/bitcoin/pull/26596#discussion_r1661582339)
(Agreed, but then I would skip doing the suggested change in the .CPP file).
👍 cbergqvist approved a pull request: "wallet: Migrate legacy wallets to descriptor wallets without requiring BDB"
(https://github.com/bitcoin/bitcoin/pull/26596#pullrequestreview-2152310896)
ACK 8ce3739edbcf6437bf2695087e0ebe8c633df19b
`git range-diff 9700c217~5..9700c217 8ce3739~5..8ce3739` - not the easiest read, also used GH [compare](https://github.com/bitcoin/bitcoin/compare/9700c21704ddad4be07df44393e39057fa386c56..8ce3739edbcf6437bf2695087e0ebe8c633df19b).
Passed `make check` & `test/functional/test_runner.py` including `p2p_handshake.py` which [failed on CI](https://github.com/bitcoin/bitcoin/pull/26596/checks?check_run_id=26906002421).
Only one non-critical reserva
...
(https://github.com/bitcoin/bitcoin/pull/26596#pullrequestreview-2152310896)
ACK 8ce3739edbcf6437bf2695087e0ebe8c633df19b
`git range-diff 9700c217~5..9700c217 8ce3739~5..8ce3739` - not the easiest read, also used GH [compare](https://github.com/bitcoin/bitcoin/compare/9700c21704ddad4be07df44393e39057fa386c56..8ce3739edbcf6437bf2695087e0ebe8c633df19b).
Passed `make check` & `test/functional/test_runner.py` including `p2p_handshake.py` which [failed on CI](https://github.com/bitcoin/bitcoin/pull/26596/checks?check_run_id=26906002421).
Only one non-critical reserva
...
🤔 cbergqvist requested changes to a pull request: "Make it possible to disable Tor binds and abort startup on bind failure"
(https://github.com/bitcoin/bitcoin/pull/22729#pullrequestreview-2152330809)
NACK ffe9b274984a351716348a6c99df19c391bfdc8e
The current solution means most test nodes in the functional test suite will now bind to an onion port even though the vast majority are not testing Tor. My proposed solution in 6b785873557696cc611d58fdcac5a3d54622082c did not have that behavior. Maybe you could add a custom flag on the Python test framework level that can be opted in to by tests that require it?
(https://github.com/bitcoin/bitcoin/pull/22729#pullrequestreview-2152330809)
NACK ffe9b274984a351716348a6c99df19c391bfdc8e
The current solution means most test nodes in the functional test suite will now bind to an onion port even though the vast majority are not testing Tor. My proposed solution in 6b785873557696cc611d58fdcac5a3d54622082c did not have that behavior. Maybe you could add a custom flag on the Python test framework level that can be opted in to by tests that require it?
💬 cbergqvist commented on pull request "Make it possible to disable Tor binds and abort startup on bind failure":
(https://github.com/bitcoin/bitcoin/pull/22729#discussion_r1661599469)
Did you skip this in the ffe9b274984a351716348a6c99df19c391bfdc8e force-push because you weren't modifying this specific file or did you forget?
(https://github.com/bitcoin/bitcoin/pull/22729#discussion_r1661599469)
Did you skip this in the ffe9b274984a351716348a6c99df19c391bfdc8e force-push because you weren't modifying this specific file or did you forget?
👍 tdb3 approved a pull request: "Revert "test: p2p: check that connecting to ourself leads to disconnect""
(https://github.com/bitcoin/bitcoin/pull/30374#pullrequestreview-2152339467)
ACK 9ec2c53701a391629b55aeb2804e8060d2c453a4
Glad this test found a bug!
Makes sense to revert this out until the issue is fixed.
Ran the test locally.
(https://github.com/bitcoin/bitcoin/pull/30374#pullrequestreview-2152339467)
ACK 9ec2c53701a391629b55aeb2804e8060d2c453a4
Glad this test found a bug!
Makes sense to revert this out until the issue is fixed.
Ran the test locally.
📝 crypto-perry opened a pull request: "Qrwit nonbc"
(https://github.com/bitcoin/bitcoin/pull/30375)
This is a draft implementation of the scheme described in this (research paper)[https://royalsocietypublishing.org/doi/10.1098/rsos.180410] which introduces a commit delay reveal scheme for safely transitioning ECDSA secured UTXOs to a quantum resistant signature scheme.
(https://github.com/bitcoin/bitcoin/pull/30375)
This is a draft implementation of the scheme described in this (research paper)[https://royalsocietypublishing.org/doi/10.1098/rsos.180410] which introduces a commit delay reveal scheme for safely transitioning ECDSA secured UTXOs to a quantum resistant signature scheme.
👋 crypto-perry's pull request is ready for review: "Qrwit nonbc"
(https://github.com/bitcoin/bitcoin/pull/30375)
(https://github.com/bitcoin/bitcoin/pull/30375)
📝 crypto-perry converted_to_draft a pull request: "QRWit Implementation Proposal"
(https://github.com/bitcoin/bitcoin/pull/30375)
This is a draft implementation of the scheme described in this [research paper](https://royalsocietypublishing.org/doi/10.1098/rsos.180410) which introduces a commit delay reveal scheme for safely transitioning ECDSA secured UTXOs to a quantum resistant signature scheme.
(https://github.com/bitcoin/bitcoin/pull/30375)
This is a draft implementation of the scheme described in this [research paper](https://royalsocietypublishing.org/doi/10.1098/rsos.180410) which introduces a commit delay reveal scheme for safely transitioning ECDSA secured UTXOs to a quantum resistant signature scheme.