💬 sdaftuar commented on pull request "policy: restrict all TRUC (v3) transactions to 25KvB":
(https://github.com/bitcoin/bitcoin/pull/29873#issuecomment-2057668820)
> Would this make things {broken, inconvenient} for potential users of TRUCs, e.g. by requiring swapping between v3 and v2?
Just wanted to add -- I think the case I expect more often would be that users would just create multiple transactions if they had a need to construct a transaction greater than 25kvb (similar to what I think everyone must do today to work around the current 100kvb standardness limit).
The additional transaction overhead from constructing four 25kvb transactions inste
...
(https://github.com/bitcoin/bitcoin/pull/29873#issuecomment-2057668820)
> Would this make things {broken, inconvenient} for potential users of TRUCs, e.g. by requiring swapping between v3 and v2?
Just wanted to add -- I think the case I expect more often would be that users would just create multiple transactions if they had a need to construct a transaction greater than 25kvb (similar to what I think everyone must do today to work around the current 100kvb standardness limit).
The additional transaction overhead from constructing four 25kvb transactions inste
...
✅ laanwj closed an issue: "v27.0 Testing"
(https://github.com/bitcoin/bitcoin/issues/29697)
(https://github.com/bitcoin/bitcoin/issues/29697)
💬 laanwj commented on issue "v27.0 Testing":
(https://github.com/bitcoin/bitcoin/issues/29697#issuecomment-2057674519)
Closing this issue now that `v27.0` final is tagged.
(https://github.com/bitcoin/bitcoin/issues/29697#issuecomment-2057674519)
Closing this issue now that `v27.0` final is tagged.
✅ fanquake closed an issue: "27.0 RC Testing Guide Feedback"
(https://github.com/bitcoin/bitcoin/issues/29685)
(https://github.com/bitcoin/bitcoin/issues/29685)
💬 achow101 commented on pull request "wallet: Implement independent BDB parser":
(https://github.com/bitcoin/bitcoin/pull/26606#discussion_r1566348693)
Dropped `uint160`, changed to use an array.
(https://github.com/bitcoin/bitcoin/pull/26606#discussion_r1566348693)
Dropped `uint160`, changed to use an array.
💬 achow101 commented on pull request "wallet: Implement independent BDB parser":
(https://github.com/bitcoin/bitcoin/pull/26606#discussion_r1566349302)
Changed to `int64_t` with casting.
(https://github.com/bitcoin/bitcoin/pull/26606#discussion_r1566349302)
Changed to `int64_t` with casting.
💬 achow101 commented on pull request "wallet: Implement independent BDB parser":
(https://github.com/bitcoin/bitcoin/pull/26606#discussion_r1566349587)
Yes, I've added a check for that.
(https://github.com/bitcoin/bitcoin/pull/26606#discussion_r1566349587)
Yes, I've added a check for that.
🤔 sr-gi reviewed a pull request: "net_processing: make any misbehavior trigger immediate discouragement"
(https://github.com/bitcoin/bitcoin/pull/29575#pullrequestreview-2001909800)
A couple of doc nits and a question below
(https://github.com/bitcoin/bitcoin/pull/29575#pullrequestreview-2001909800)
A couple of doc nits and a question below
💬 sr-gi commented on pull request "net_processing: make any misbehavior trigger immediate discouragement":
(https://github.com/bitcoin/bitcoin/pull/29575#discussion_r1566280997)
in: 78515e185755f5eba951917b8e2314ec69446d7d
nit: isn't this 99? (`for i in range(1, NUM_HEADERS)`)
(https://github.com/bitcoin/bitcoin/pull/29575#discussion_r1566280997)
in: 78515e185755f5eba951917b8e2314ec69446d7d
nit: isn't this 99? (`for i in range(1, NUM_HEADERS)`)
💬 sr-gi commented on pull request "net_processing: make any misbehavior trigger immediate discouragement":
(https://github.com/bitcoin/bitcoin/pull/29575#discussion_r1566343004)
in: ce238dd79bd3a1190a2229fac4502fa5742fc6e3
I think the comment 6 lines below is wrong now:
```
// It should be impossible for the getheaders request to fail,
// because we should have cleared the last getheaders timestamp
// when processing the headers that triggered this call.
[...]
```
IIUC, `result.request_more` cannot be true if `result.success` is false (based on my understanding of [HeadersSyncState::ProcessNextHeaders](https://github.com/bitcoin/bitcoin/blob/07720b1cdd773
...
(https://github.com/bitcoin/bitcoin/pull/29575#discussion_r1566343004)
in: ce238dd79bd3a1190a2229fac4502fa5742fc6e3
I think the comment 6 lines below is wrong now:
```
// It should be impossible for the getheaders request to fail,
// because we should have cleared the last getheaders timestamp
// when processing the headers that triggered this call.
[...]
```
IIUC, `result.request_more` cannot be true if `result.success` is false (based on my understanding of [HeadersSyncState::ProcessNextHeaders](https://github.com/bitcoin/bitcoin/blob/07720b1cdd773
...
💬 sr-gi commented on pull request "net_processing: make any misbehavior trigger immediate discouragement":
(https://github.com/bitcoin/bitcoin/pull/29575#discussion_r1566293700)
in: ce238dd79bd3a1190a2229fac4502fa5742fc6e3
nit: I found this wording a bit hard to follow, especially the "to make marked responded to" bit.
IIUC, this is sending an empty block as a response to the outstanding getheaders request from the previous loop interaction, otherwise sending `blocks[i]` would be treated as the reply instead. Is that it? If so, I'd suggest something along the lines of:
```suggestion
# Send an empty header as a failed response to the received gethea
...
(https://github.com/bitcoin/bitcoin/pull/29575#discussion_r1566293700)
in: ce238dd79bd3a1190a2229fac4502fa5742fc6e3
nit: I found this wording a bit hard to follow, especially the "to make marked responded to" bit.
IIUC, this is sending an empty block as a response to the outstanding getheaders request from the previous loop interaction, otherwise sending `blocks[i]` would be treated as the reply instead. Is that it? If so, I'd suggest something along the lines of:
```suggestion
# Send an empty header as a failed response to the received gethea
...
💬 achow101 commented on pull request "wallet: Implement independent BDB parser":
(https://github.com/bitcoin/bitcoin/pull/26606#issuecomment-2057691808)
> parsing the headers of overflow pages (which have btree level 0)
Interesting, did not know that they would use an invalid btree level. Fixed as suggested.
***
I've also commented out the enums that we do not use. I decided to leave them there for completeness, but they won't be used.
***
For help with reviewing, I suggest looking at the BDB source code. I've uploaded a copy of the version that we use at https://github.com/achow101/bdb-sources/tree/main/4.8.30.NC. The files that
...
(https://github.com/bitcoin/bitcoin/pull/26606#issuecomment-2057691808)
> parsing the headers of overflow pages (which have btree level 0)
Interesting, did not know that they would use an invalid btree level. Fixed as suggested.
***
I've also commented out the enums that we do not use. I decided to leave them there for completeness, but they won't be used.
***
For help with reviewing, I suggest looking at the BDB source code. I've uploaded a copy of the version that we use at https://github.com/achow101/bdb-sources/tree/main/4.8.30.NC. The files that
...
💬 laanwj commented on issue "utils: wallet_dump can create a `database` directory, cross-pollinating records":
(https://github.com/bitcoin/bitcoin/issues/29883#issuecomment-2057708243)
Oh, i think part of the trigger here is that some of the wallets are backups of the same database at different times, so the logs can get re-used because the unique id matches.
(https://github.com/bitcoin/bitcoin/issues/29883#issuecomment-2057708243)
Oh, i think part of the trigger here is that some of the wallets are backups of the same database at different times, so the logs can get re-used because the unique id matches.
💬 laanwj commented on pull request "netbase: clean up Proxy logging":
(https://github.com/bitcoin/bitcoin/pull/29882#discussion_r1566368437)
It's probably a good idea to keep the `!IsValid` check here, just to be sure. Fine with removing the log.
(https://github.com/bitcoin/bitcoin/pull/29882#discussion_r1566368437)
It's probably a good idea to keep the `!IsValid` check here, just to be sure. Fine with removing the log.
💬 maflcko commented on pull request "build: Fix false positive `CHECK_ATOMIC` test":
(https://github.com/bitcoin/bitcoin/pull/29859#issuecomment-2057777762)
Ok, I could reproduce by setting this CI to lunar: `FILE_ENV="./ci/test/00_setup_env_i686_multiprocess.sh" ./ci/test_run_all.sh`. However, on jammy, mantic, and noble it passes.
(https://github.com/bitcoin/bitcoin/pull/29859#issuecomment-2057777762)
Ok, I could reproduce by setting this CI to lunar: `FILE_ENV="./ci/test/00_setup_env_i686_multiprocess.sh" ./ci/test_run_all.sh`. However, on jammy, mantic, and noble it passes.
💬 achow101 commented on pull request "wallet: Implement independent BDB parser":
(https://github.com/bitcoin/bitcoin/pull/26606#issuecomment-2057799622)
There was a comment at CoreDev that this should check that the file is compacted, so I've added a commit that does that. To check that this is correct, see the implementation of [`lsn_reset`](https://github.com/achow101/bdb-sources/blob/main/4.8.30.NC/db/db_setlsn.c#L114), which uses [`LSN_NOT_LOGGED`](https://github.com/achow101/bdb-sources/blob/main/4.8.30.NC/dbinc/db_int.in#L803). The definition of [`DB_LSN`](https://github.com/achow101/bdb-sources/blob/main/4.8.30.NC/dbinc/db.in#L444) is als
...
(https://github.com/bitcoin/bitcoin/pull/26606#issuecomment-2057799622)
There was a comment at CoreDev that this should check that the file is compacted, so I've added a commit that does that. To check that this is correct, see the implementation of [`lsn_reset`](https://github.com/achow101/bdb-sources/blob/main/4.8.30.NC/db/db_setlsn.c#L114), which uses [`LSN_NOT_LOGGED`](https://github.com/achow101/bdb-sources/blob/main/4.8.30.NC/dbinc/db_int.in#L803). The definition of [`DB_LSN`](https://github.com/achow101/bdb-sources/blob/main/4.8.30.NC/dbinc/db.in#L444) is als
...
⚠️ hebasto opened an issue: "`test/streams_tests.cpp` fails to compile on SunOS / illumos"
(https://github.com/bitcoin/bitcoin/issues/29884)
While testing bitcoin/bitcoin#29484, which seems not related to this bug, I've notice the following failure:
```
$ ./autogen.sh
$ ./configure
$ gmake -C src test/test_bitcoin
...
CXX test/test_bitcoin-streams_tests.o
In file included from test/streams_tests.cpp:5:
./streams.h: In instantiation of 'SpanReader& SpanReader::operator>>(T&&) [with T = signed char&]':
test/streams_tests.cpp:148:15: required from here
./streams.h:114:22: error: no matching function for call to 'Unser
...
(https://github.com/bitcoin/bitcoin/issues/29884)
While testing bitcoin/bitcoin#29484, which seems not related to this bug, I've notice the following failure:
```
$ ./autogen.sh
$ ./configure
$ gmake -C src test/test_bitcoin
...
CXX test/test_bitcoin-streams_tests.o
In file included from test/streams_tests.cpp:5:
./streams.h: In instantiation of 'SpanReader& SpanReader::operator>>(T&&) [with T = signed char&]':
test/streams_tests.cpp:148:15: required from here
./streams.h:114:22: error: no matching function for call to 'Unser
...
💬 mzumsande commented on pull request "p2p: opportunistically accept 1-parent-1-child packages":
(https://github.com/bitcoin/bitcoin/pull/28970#discussion_r1566233731)
Should update `ProcessInvalidTx` doc above too (it lists all member variables that the function updates).
(https://github.com/bitcoin/bitcoin/pull/28970#discussion_r1566233731)
Should update `ProcessInvalidTx` doc above too (it lists all member variables that the function updates).
🤔 mzumsande reviewed a pull request: "p2p: opportunistically accept 1-parent-1-child packages"
(https://github.com/bitcoin/bitcoin/pull/28970#pullrequestreview-2001371851)
Started reviewing - haven't looked at the tests yet.
(https://github.com/bitcoin/bitcoin/pull/28970#pullrequestreview-2001371851)
Started reviewing - haven't looked at the tests yet.
💬 mzumsande commented on pull request "p2p: opportunistically accept 1-parent-1-child packages":
(https://github.com/bitcoin/bitcoin/pull/28970#discussion_r1566300445)
have you considered returning early here if `cpfp_candidates` is empty? It should work regardless, but seems conceptually simpler than checking all the steps below can handle that case (and may be slightly faster too).
(https://github.com/bitcoin/bitcoin/pull/28970#discussion_r1566300445)
have you considered returning early here if `cpfp_candidates` is empty? It should work regardless, but seems conceptually simpler than checking all the steps below can handle that case (and may be slightly faster too).