💬 ryanofsky commented on pull request "refactor: Remove CAddressBookData::destdata":
(https://github.com/bitcoin/bitcoin/pull/27224#discussion_r1149661885)
re: https://github.com/bitcoin/bitcoin/pull/27224#discussion_r1149193825
Thanks, switched to brace initialization like you suggested in many places now. Should be clearer and safer
(https://github.com/bitcoin/bitcoin/pull/27224#discussion_r1149661885)
re: https://github.com/bitcoin/bitcoin/pull/27224#discussion_r1149193825
Thanks, switched to brace initialization like you suggested in many places now. Should be clearer and safer
💬 Ayush170-Future commented on pull request "ci: cleanup of CI_EXEC & CI_EXEC_ROOT (refs #27321)":
(https://github.com/bitcoin/bitcoin/pull/27333#issuecomment-1485729334)
ACK on the concept.
I've been following this since the start and have reviewed the changes once again. The approach followed looks good to me. If the checks fail again, I'd be happy to re-review (they are running currently).
(https://github.com/bitcoin/bitcoin/pull/27333#issuecomment-1485729334)
ACK on the concept.
I've been following this since the start and have reviewed the changes once again. The approach followed looks good to me. If the checks fail again, I'd be happy to re-review (they are running currently).
💬 ryanofsky commented on pull request "RPC: Accept options as named-only parameters":
(https://github.com/bitcoin/bitcoin/pull/26485#issuecomment-1485740590)
Rebased 7fd1401d8116323adfa2a87bbcf6ea41437cd0fa -> 927a910cbe6c96d60c6a8b05baad179ef3f61f1c ([`pr/nonly.11`](https://github.com/ryanofsky/bitcoin/commits/pr/nonly.11) -> [`pr/nonly.12`](https://github.com/ryanofsky/bitcoin/commits/pr/nonly.12), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/nonly.11-rebase..pr/nonly.12)) due to conflict with #26642.
(https://github.com/bitcoin/bitcoin/pull/26485#issuecomment-1485740590)
Rebased 7fd1401d8116323adfa2a87bbcf6ea41437cd0fa -> 927a910cbe6c96d60c6a8b05baad179ef3f61f1c ([`pr/nonly.11`](https://github.com/ryanofsky/bitcoin/commits/pr/nonly.11) -> [`pr/nonly.12`](https://github.com/ryanofsky/bitcoin/commits/pr/nonly.12), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/nonly.11-rebase..pr/nonly.12)) due to conflict with #26642.
📝 mzumsande opened a pull request: "test: fix intermittent failure in ChainStateManager tests"
(https://github.com/bitcoin/bitcoin/pull/27348)
Before wiping a `ChainStateManager` and creating a new one, the `validationinterface` queue should be emptied to avoid the possibility of accessing deleted memory.
This could lead to very rare CI failures reported in #26613 and #27320 (see [here](https://github.com/bitcoin/bitcoin/issues/27320#issuecomment-1485694691) for a more detailed explanation).
Fixes #27320
(https://github.com/bitcoin/bitcoin/pull/27348)
Before wiping a `ChainStateManager` and creating a new one, the `validationinterface` queue should be emptied to avoid the possibility of accessing deleted memory.
This could lead to very rare CI failures reported in #26613 and #27320 (see [here](https://github.com/bitcoin/bitcoin/issues/27320#issuecomment-1485694691) for a more detailed explanation).
Fixes #27320
💬 jamesob commented on pull request "test: fix intermittent failure in ChainStateManager tests":
(https://github.com/bitcoin/bitcoin/pull/27348#issuecomment-1485769153)
crACK https://github.com/bitcoin/bitcoin/pull/27348/commits/f8abcb3e3b2e731c002ec88f7559c21e26a2c079
Thank you for fixing this! During *actual* node restart (which this unittest is trying to model) the validationinterface queue will of course have been flushed, so this change makes the test affected more realistic.
(https://github.com/bitcoin/bitcoin/pull/27348#issuecomment-1485769153)
crACK https://github.com/bitcoin/bitcoin/pull/27348/commits/f8abcb3e3b2e731c002ec88f7559c21e26a2c079
Thank you for fixing this! During *actual* node restart (which this unittest is trying to model) the validationinterface queue will of course have been flushed, so this change makes the test affected more realistic.
💬 amitiuttarwar commented on pull request "addrman: Enable selecting addresses by network":
(https://github.com/bitcoin/bitcoin/pull/27214#discussion_r1149737863)
> Try to asses how much is the slowdown. Use a full addrman (15k is not full) because that is what people out there are running on. If the slowdown is ok, then no further improvements are necessary on this PR or its parent.
agreed that this is the fundamental thing we are trying to evaluate - is the performance difference significant & acceptable
> I will try to compare on a snapshot of an addrman from my public full node, not on artificially filled addrman from the bench. Last time I chec
...
(https://github.com/bitcoin/bitcoin/pull/27214#discussion_r1149737863)
> Try to asses how much is the slowdown. Use a full addrman (15k is not full) because that is what people out there are running on. If the slowdown is ok, then no further improvements are necessary on this PR or its parent.
agreed that this is the fundamental thing we are trying to evaluate - is the performance difference significant & acceptable
> I will try to compare on a snapshot of an addrman from my public full node, not on artificially filled addrman from the bench. Last time I chec
...
💬 mzumsande commented on pull request "addrman: Enable selecting addresses by network":
(https://github.com/bitcoin/bitcoin/pull/27214#discussion_r1149790004)
> What is your "Test 5" doing? Does it call 20 times `Select(network)`?
No, it calls `Select(network)` for an addrman that has 20 addresses of the selected network.
I'll try to explain the idea behind my benchmarks better:
Starting point is the observation that the suggested change makes `Select` slower if we already know a lot of addresses of the desired type, while making it faster if addrman has very few addresses of this type. This is true for botch `Select()` and `Select(network)`,
...
(https://github.com/bitcoin/bitcoin/pull/27214#discussion_r1149790004)
> What is your "Test 5" doing? Does it call 20 times `Select(network)`?
No, it calls `Select(network)` for an addrman that has 20 addresses of the selected network.
I'll try to explain the idea behind my benchmarks better:
Starting point is the observation that the suggested change makes `Select` slower if we already know a lot of addresses of the desired type, while making it faster if addrman has very few addresses of this type. This is true for botch `Select()` and `Select(network)`,
...
💬 willcl-ark commented on pull request "init: Error if ignored bitcoin.conf file is found":
(https://github.com/bitcoin/bitcoin/pull/27302#discussion_r1149811002)
I don't know if I'm doing something supported here, but this doesn't seem to work for me:
```fish
./src/bitcoind -datadir=/tmp/bitcoin_27302/ -rpcport=18555 -port=18556
```
```log
2023-03-27T21:22:09Z Default data directory /home/will/.bitcoin
2023-03-27T21:22:09Z Using data directory /tmp/bitcoin_27302/regtest
2023-03-27T21:22:09Z Config file: /tmp/bitcoin_27302/bitcoin.conf
2023-03-27T21:22:09Z Config file arg: datadir="/tmp/bitcoin_27302/datadir2"
2023-03-27T21:22:09Z Config file
...
(https://github.com/bitcoin/bitcoin/pull/27302#discussion_r1149811002)
I don't know if I'm doing something supported here, but this doesn't seem to work for me:
```fish
./src/bitcoind -datadir=/tmp/bitcoin_27302/ -rpcport=18555 -port=18556
```
```log
2023-03-27T21:22:09Z Default data directory /home/will/.bitcoin
2023-03-27T21:22:09Z Using data directory /tmp/bitcoin_27302/regtest
2023-03-27T21:22:09Z Config file: /tmp/bitcoin_27302/bitcoin.conf
2023-03-27T21:22:09Z Config file arg: datadir="/tmp/bitcoin_27302/datadir2"
2023-03-27T21:22:09Z Config file
...
💬 Xekyo commented on pull request "Implement Mini version of BlockAssembler to calculate mining scores":
(https://github.com/bitcoin/bitcoin/pull/27021#discussion_r1149709427)
Since I’ve removed the special casing that makes us bail if we start with 500 in the first place, I have left it at the start of the loop.
(https://github.com/bitcoin/bitcoin/pull/27021#discussion_r1149709427)
Since I’ve removed the special casing that makes us bail if we start with 500 in the first place, I have left it at the start of the loop.
💬 Xekyo commented on pull request "Implement Mini version of BlockAssembler to calculate mining scores":
(https://github.com/bitcoin/bitcoin/pull/27021#discussion_r1149709631)
I’ve adopted your suggestion, thanks
(https://github.com/bitcoin/bitcoin/pull/27021#discussion_r1149709631)
I’ve adopted your suggestion, thanks
💬 Xekyo commented on pull request "Implement Mini version of BlockAssembler to calculate mining scores":
(https://github.com/bitcoin/bitcoin/pull/27021#discussion_r1149712008)
Right, if txids is empty, we already return an empty vector anyway, and the 500 limit is already checked at the start of the loop. Sorry, @furszy, I changed my mind about [your prior suggestion to add an early exit](https://github.com/bitcoin/bitcoin/pull/27021/files#diff-c065d4cd2398ad0dbcef393c5dfc53f465bf44723348892395fffd2fb3bac522).
(https://github.com/bitcoin/bitcoin/pull/27021#discussion_r1149712008)
Right, if txids is empty, we already return an empty vector anyway, and the 500 limit is already checked at the start of the loop. Sorry, @furszy, I changed my mind about [your prior suggestion to add an early exit](https://github.com/bitcoin/bitcoin/pull/27021/files#diff-c065d4cd2398ad0dbcef393c5dfc53f465bf44723348892395fffd2fb3bac522).
💬 Xekyo commented on pull request "Implement Mini version of BlockAssembler to calculate mining scores":
(https://github.com/bitcoin/bitcoin/pull/27021#discussion_r1149422663)
See response on prior comment
(https://github.com/bitcoin/bitcoin/pull/27021#discussion_r1149422663)
See response on prior comment
💬 Xekyo commented on pull request "Implement Mini version of BlockAssembler to calculate mining scores":
(https://github.com/bitcoin/bitcoin/pull/27021#discussion_r1149659725)
Removed unimplemented function
(https://github.com/bitcoin/bitcoin/pull/27021#discussion_r1149659725)
Removed unimplemented function
💬 Xekyo commented on pull request "Implement Mini version of BlockAssembler to calculate mining scores":
(https://github.com/bitcoin/bitcoin/pull/27021#discussion_r1149445980)
Yeah, it felt safer to first update everything, then to start deleting things. I did try combining the loops and it did not break anything, though. Still deciding, maybe this could be a follow-up.
(https://github.com/bitcoin/bitcoin/pull/27021#discussion_r1149445980)
Yeah, it felt safer to first update everything, then to start deleting things. I did try combining the loops and it did not break anything, though. Still deciding, maybe this could be a follow-up.
💬 Xekyo commented on pull request "Implement Mini version of BlockAssembler to calculate mining scores":
(https://github.com/bitcoin/bitcoin/pull/27021#discussion_r1149399579)
I am not sure I follow. For historical reasons, transactions are included in their own ancestor set and their own descendant set. So while we are iterating over descendants, we could be evaluating a transaction itself that has no ancestors left after they were picked into our “virtual block”. If the evaluated transaction doesn’t have ancestors left, and we get its size with ancestors, we are comparing its own size with its own size in this inequality. Therefore, same size must be permitted.
I
...
(https://github.com/bitcoin/bitcoin/pull/27021#discussion_r1149399579)
I am not sure I follow. For historical reasons, transactions are included in their own ancestor set and their own descendant set. So while we are iterating over descendants, we could be evaluating a transaction itself that has no ancestors left after they were picked into our “virtual block”. If the evaluated transaction doesn’t have ancestors left, and we get its size with ancestors, we are comparing its own size with its own size in this inequality. Therefore, same size must be permitted.
I
...
💬 Xekyo commented on pull request "Implement Mini version of BlockAssembler to calculate mining scores":
(https://github.com/bitcoin/bitcoin/pull/27021#discussion_r1149653633)
Yeah, agreed. Gonna remove it
(https://github.com/bitcoin/bitcoin/pull/27021#discussion_r1149653633)
Yeah, agreed. Gonna remove it
💬 Xekyo commented on pull request "Implement Mini version of BlockAssembler to calculate mining scores":
(https://github.com/bitcoin/bitcoin/pull/27021#discussion_r1149782804)
I’ve renamed the variable and better explained what’s going on here.
(https://github.com/bitcoin/bitcoin/pull/27021#discussion_r1149782804)
I’ve renamed the variable and better explained what’s going on here.
💬 Xekyo commented on pull request "Implement Mini version of BlockAssembler to calculate mining scores":
(https://github.com/bitcoin/bitcoin/pull/27021#discussion_r1149731957)
I’ve removed `all_entries`. Thanks for catching that
(https://github.com/bitcoin/bitcoin/pull/27021#discussion_r1149731957)
I’ve removed `all_entries`. Thanks for catching that
💬 willcl-ark commented on pull request "init: Error if ignored bitcoin.conf file is found":
(https://github.com/bitcoin/bitcoin/pull/27302#discussion_r1149825850)
I also do not get the error with:
```fish
₿ ./src/bitcoind -datadir=/tmp/bitcoin_27302/ -conf=/tmp/bitcoin_27302/bitcoin.conf -rpcport=18555 -port=18556
```
But I do see the error (as described in the commit bullet point 2) with:
```fish
₿ ./src/bitcoind -conf=/tmp/bitcoin_27302/bitcoin.conf -rpcport=18555 -port=18556
```
In my opinion:
> Show an error on startup if a bitcoin datadir that is being used contains a
> `bitcoin.conf` file that is ignored. There are two cases wher
...
(https://github.com/bitcoin/bitcoin/pull/27302#discussion_r1149825850)
I also do not get the error with:
```fish
₿ ./src/bitcoind -datadir=/tmp/bitcoin_27302/ -conf=/tmp/bitcoin_27302/bitcoin.conf -rpcport=18555 -port=18556
```
But I do see the error (as described in the commit bullet point 2) with:
```fish
₿ ./src/bitcoind -conf=/tmp/bitcoin_27302/bitcoin.conf -rpcport=18555 -port=18556
```
In my opinion:
> Show an error on startup if a bitcoin datadir that is being used contains a
> `bitcoin.conf` file that is ignored. There are two cases wher
...
📝 ismaelsadeeq opened a pull request: "refactor: use address_to_scriptpubkey instead of RPC call"
(https://github.com/bitcoin/bitcoin/pull/27349)
PR #27269 enables the function address_to_scriptpubkey() to decode all address types and return their corresponding scriptpubkeys. As a result, there is no longer any need to call getaddressinfo or validateaddress RPCs in order to retrieve an address scriptpubkey, as explained in the comments on this pull request (see https://github.com/bitcoin/bitcoin/pull/27269#pullrequestreview-1353681933 and https://github.com/bitcoin/bitcoin/pull/27269#issuecomment-1481016118).
Instead of using RPC calls
...
(https://github.com/bitcoin/bitcoin/pull/27349)
PR #27269 enables the function address_to_scriptpubkey() to decode all address types and return their corresponding scriptpubkeys. As a result, there is no longer any need to call getaddressinfo or validateaddress RPCs in order to retrieve an address scriptpubkey, as explained in the comments on this pull request (see https://github.com/bitcoin/bitcoin/pull/27269#pullrequestreview-1353681933 and https://github.com/bitcoin/bitcoin/pull/27269#issuecomment-1481016118).
Instead of using RPC calls
...
💬 ismaelsadeeq commented on pull request "refactor: use address_to_scriptpubkey instead of RPC call":
(https://github.com/bitcoin/bitcoin/pull/27349#issuecomment-1485964435)
@josibake @theStack @willcl-ark this is PR https://github.com/bitcoin/bitcoin/pull/27269 follow up, I will appreciate your reviews.
Thank you
(https://github.com/bitcoin/bitcoin/pull/27349#issuecomment-1485964435)
@josibake @theStack @willcl-ark this is PR https://github.com/bitcoin/bitcoin/pull/27269 follow up, I will appreciate your reviews.
Thank you